Merge lp:~openerp-community/openobject-addons/lpistone_6.0_lp784499 into lp:openobject-addons/6.0

[Leonardo Pistone]

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

Hi Leonardo,

Thanks for the bug report and the merge proposal!

There is one thing that could be improved in your patch: instead of using a direct SQL query and having to manually filter on the company, it would be much better to rely on the use of ORM methods, which do perform the appropriate filtering (due to the record rules defined in ir.rules).
This way you don't hardcode the filtering, and you also avoid bypassing the ORM, which is not recommended.

If the initial code had done the same, there would have been no bug, so let's use this opportunity to improve it :-)

E.g:
  account_ids = self.pool.get('account.account').search(cr, uid,[('parent_id', '=', False)], context=context, limit=1)
  return account_ids[0] if account_ids else False

review: Needs Fixing

[Fabien (Open ERP) (fp-tinyerp)]

Olivier, to be confirmed, but I think company_id=%d is still required;
in case of an hierarchy of companies as I suppose the rule uses child_of
instead of =

Olivier Dony (OpenERP) wrote:
> Review: Needs Fixing
> Hi Leonardo,
>
> Thanks for the bug report and the merge proposal!
>
> There is one thing that could be improved in your patch: instead of using a direct SQL query and having to manually filter on the company, it would be much better to rely on the use of ORM methods, which do perform the appropriate filtering (due to the record rules defined in ir.rules).
> This way you don't hardcode the filtering, and you also avoid bypassing the ORM, which is not recommended.
>
> If the initial code had done the same, there would have been no bug, so let's use this opportunity to improve it :-)
>
> E.g:
> account_ids = self.pool.get('account.account').search(cr, uid,[('parent_id', '=', False)], context=context, limit=1)
> return account_ids[0] if account_ids else False

--
Fabien Pinckaers
CEO OpenERP
Chaussée de Namur 40
B-1367 Grand-Rosière
Belgium
Phone: [phone]
Fax: [phone]
Web: http://openerp.com

[Leonardo Pistone (lepistone)]

Olivier, Fabien,

I agree with you both.

In general, there are many situations where there are sql queries that break with access rules and/or company_ids. Many of these can be converted to the ORM way and do the right thing transparently, with the exception of the ones that would be too slow (that is not the case here).

Still, that bug is a bit different: we are talking about the account_id of a decoration invoice line. The account_id doesn't make sense, and in fact isn't shown to the user, and is there just because that field is required in the invoice line. So our query should get the account_id that is least likely to be restricted/changed in the future. That's probably why the original code looks for the root account.

Example: in a multi-company setup, a user could have access to all accounts, but still it is nicer to choose his own company's account instead of a random one.

So: I'm for Olivier's ORM way + [('company_id'),('='),(current_company)]

Thanks!

[Anup(SerpentCS) (anup-serpent)]

Hello Everyone,

  As per the discussion I have fixed the hard coded query and used the orm search method for the parent account of the company.

  I agree with Leonardo that the account chosen should be of the current company. IMHO LIMIT 1 in query may give a random result of any of the accounts available.

Please share your views.

Thanks.

[Leonardo Pistone (lepistone)]

Anup,

I tested your patch with my customer's database that was breaking in the first place, and it looks ok to me.

Best

review: Approve

[Leonardo Pistone (lepistone)]

Jay,

that patch seems OK to us, can we merge it?

thanks a lot

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

review: Approve

[Leonardo Pistone (lepistone)]

Hi,

there is something wrong going on here. If you browse the code:

http://bazaar.launchpad.net/~openerp/openobject-addons/6.0/view/head:/account_invoice_layout/account_invoice_layout.py#L167

the new code is not there. That confuses me ). What happened?

Thanks!

[Anup(SerpentCS) (anup-serpent)]

Hello Leonardo,

It was already merged at revision 4617 in stable v6.

Check out the following. http://bazaar.launchpad.net/~openerp/openobject-addons/6.0/revision/4594.2.1

There seems to be some problem with launchpad. I have fixed it again with revision 4628 <email address hidden>

Thanks.

=== modified file 'account_invoice_layout/account_invoice_layout.py'
--- account_invoice_layout/account_invoice_layout.py	2011-01-14 00:11:01 +0000
+++ account_invoice_layout/account_invoice_layout.py	2011-05-20 12:11:33 +0000
@@ -165,9 +165,11 @@
     }
 
     def _default_account(self, cr, uid, context=None):
-        cr.execute("select id from account_account where parent_id IS NULL LIMIT 1")
-        res = cr.fetchone()
-        return res[0]
+        if context is None:
+            context = {}
+        current_company = self.pool.get('res.users').browse(cr,uid,uid).company_id.id
+        account_id = self.pool.get('account.account').search(cr, uid, [('company_id','=',current_company),('parent_id','=',False)], limit=1,context=context)
+        return account_id and account_id[0] or False
 
     _defaults = {
         'state': 'article',