openerp-dev team mailing list archive

["Stéphane Wirtel, OpenERP" <stw@xxxxxxxxxxx>]

Hi all,

Here is a part of the last commit in the point_of_sale module and I don't like that.

[code]
cr.execute(""" select id from account_journal
            where auto_cash='True' and type='cash'
            and id in (%s)""" %(','.join(map(lambda x: "'" + str(x) + "'", j_ids))))
journal_ids = map(lambda x1: x1[0], cr.fetchall())

for journal in journal_obj.browse(cr, uid, journal_ids):
	# do something
[/code]

Remarks:
========
1. We don't use the parameters of the execute method from Psycopg2 [BAD]
-> With these parameters we can avoid the sql injections

cr.execute(""" select id from account_journal
            where auto_cash='True' and type='cash'
            and id in (%s)""" %(','.join(map(lambda x: "'" + str(x) + "'", j_ids))))

Why do you use the join and the map functions ? If you try to write this code,
we risk to get some errors during the run-time because we will introduce some sql injections,
and it's bad :/

2. We use an SQL statement instead of the search method ? In this case, I don't find the reason. [BAD]

3. We bypass the ir.rules mechanism and we risk to have a problem with the security. [BAD]

Please try to be smart. We have guidelines [1], try to respect them.

We can rewrite the query with the search method and with this way, avoid the sql query and the code is readable.
Example:

	journal_obj = self.pool.get('account.journal')
	journal_ids = journal_obj.search(cr, uid,
					[('auto_cash', '=', True),
					 ('type', '=', 'cash'),
					 ('id', 'in', j_ids)])

	for journal in journal_obj(cr, uid, journal_ids):
		# do something

4. A commented code should be removed, we have a system to restore an old part code (Bazaar) !

[1] Guidelines: http://doc.openerp.com/contribute/15_guidelines/

Quentin, Please, Could you ask to your developers to respect the guidelines.
I'm sorry but It's not very funny to read this kind of code !


Regards,

Stéphane
-- 
Stephane Wirtel - "As OpenERP is OpenSource, please feel free to contribute."
Quality/Release Manager
Technical Project Manager
OpenERP S.A.
Chaussee de Namur, 40
B-1367 Grand-Rosière
Tel: +32.81.81.37.00
Web: http://www.openerp.com
Planet: http://www.openerp.com/planet/
Blog: http://stephane-wirtel-at-tiny.blogspot.com

begin:vcard
fn;quoted-printable:St=C3=A9phane Wirtel
n;quoted-printable:Wirtel;St=C3=A9phane
org:OpenERP S.A.
adr;quoted-printable;quoted-printable:;;Chauss=C3=A9e de Namur, 40;Grand-Rosi=C3=A8re;;1367;Belgium
email;internet:stw@xxxxxxxxxxx
title:Developer
tel;work:+32.81.81.37.00
note;quoted-printable:OpenERP is an Open Source enterprise management software=0D=0A=
	=0D=0A=
	http://www.openerp.com
url:http://www.openerp.com
version:2.1
end:vcard