Merge lp:~openerp-dev/openobject-addons/7.0-opw-589931-rgo into lp:openobject-addons/7.0

[Ravi Gohil (OpenERP)]

Hi,

Steps to reproduce:
1) Install audittrail module,
2) Goto "Reporting/Audit/Audit Rules" and create a rule for an object and check "Log Deletes" and subscribe,
3) Goto that object and try to delete any of record,

You will face traceback saying,
...
File "/home/rgo/workspace/stable-7.0/openobject-addons/audittrail/audittrail.py", line 462, in process_data
    name = pool.get(model.model).name_get(cr, uid, [resource_id])[0][1]
IndexError: list index out of range

The issue is faced as the "unlink" was called prior for the resource record([resource_id]) and then "name_get" was performed on same resource_id, so name_get returns empty list.

So, I fixed this issue by taking name from "old_values" dictionary if method is "unlink".

Kindly review the fix.

Thanks.

[Csaba TOTH (tsabi)]

Hi,
i think calling the name_get and queriying the name field is not the same. If i create a table and define name_get (there is no name field), this way it won't called, and querying the name field will return empty or gone to error.
I think you need to move this query before the unlink method.

I am so sorry if i am wrong, i didn't tested the code, just looked at it!

review: Disapprove

[Naresh(OpenERP) (nch-openerp)]

Hello Csaba,

The query is ok and does not need to move it up ! As Old values i.e before the unlink of the resource ID will be stored in the old_values dictionary. The only thing that needs to be updated here in the code is that instead of directly querying for *name* it should be queried for _rec_name. This will cover the possibility of whether the *name* field is present or not.

@ravi can you improve this !

Thanks,
Naresh Soni
OpenERP Enterprise Services

[Ravi Gohil (OpenERP) (rgo-openerp)]

Hi,

I've updated the branch as per the suggestion and also integrated the code that will fix the issue mentioned in bug report: lp:1157497

Would you please review it again.

Thanks.

[Naresh(OpenERP) (nch-openerp)]

review: Approve

[Lithin T (lithint-mail)]

review: Approve

[Lithin T (lithint-mail)]

This is working fine and Fixed the issue.

review: Approve

[Leonardo Pistone (lepistone)]

Tested on runbot, I enabled an audit rule on write res.users, and tried to edit a user. The log is written correctly.

The branch that I requested to build on runbot is marked red, but the error looks like a connection error for the mailserver. We should probably fix that first befose merging.

So I would approve the code and the manual test, my "needs fixing" is only about the (probably unrelated) red light on runbot.

Thanks!

review: Needs Fixing (code review, manual test ok but runbot is red)

[Leonardo Pistone (lepistone)]

On a sidenote, I would like to propose that same fix on the 6.1 OCB branches.
I suppose not, but if someone from OpenERP SA would consider a proposal in the official 6.1 branch, I can do that instead.

Thanks!

[Ravi Gohil (OpenERP) (rgo-openerp)]

Hi Leonardo Pistone,

I merged the code with latest revision and rebuild the branch, now the runbot is green.

For V6.1, I did not create the branch as only fixes for blocking issues are to be integrated in it, but you can use the patch I created for V6.1, https://launchpadlibrarian.net/145154114/audittrail_res_user6.1.patch and if you want, you can create a branch for V6.1 to test it on runbot.

Thanks.

[Leonardo Pistone (lepistone)]

Thanks Ravi!

review: Approve (green runbot, thanks to ravi)

[Leonardo Pistone (lepistone)]

Indeed I saw your patch and I know about the policy about 6.1. Being a little lazy :) I was trying to see in advance if there is any chance of merging that in 6.1. The point is that it is a blocking bug (i.e. there is no workaround), but that is a rather obscure feature (audit trail of users).

Non problem though, I will do the MP anyway and we can discuss there. If it is not approved, I will propose it on OCB.

Thanks!

[Martin Trigaux (OpenERP) (mat-openerp)]

Hello,

This was fixed in another way (processing the data before removing) at revision 9808.
I will reject this one then.

=== modified file 'audittrail/audittrail.py'
--- audittrail/audittrail.py	2012-12-17 15:46:28 +0000
+++ audittrail/audittrail.py	2013-10-23 05:16:21 +0000
@@ -331,7 +331,7 @@
             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'):
+                if field in ('__last_update', 'id') or not resource_pool._all_columns.get(field):
                     continue
                 values[field] = resource[field]
                 # get the textual value of that field for this record
@@ -459,7 +459,11 @@
 
             # if at least one modification has been found
             for model_id, resource_id in lines:
-                name = pool.get(model.model).name_get(cr, uid, [resource_id])[0][1]
+                current_model = pool.get('ir.model').browse(cr, uid, model_id).model
+                if method == 'unlink':
+                    name = old_values[(model_id, resource_id)]['text'].get(pool.get(current_model)._rec_name)
+                else:
+                    name = pool.get(current_model).name_get(cr, uid, [resource_id])[0][1]
                 vals = {
                     'method': method,
                     'object_id': model_id,