Code review comment for lp:~openerp-dev/openobject-addons/trunk-bug-832635-nch

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