Convert Sale Module to Transaction system

[pheller, 2010-08-04 19:46:08]

Ok, my first shot at sale.

[ced, 2010-08-04 21:48:05]

http://codereview.appspot.com/1892048/diff/3001/4002
File sale.py (right):

http://codereview.appspot.com/1892048/diff/3001/4002#newcode145
sale.py:145: Transaction().cursor.execute("UPDATE ir_model_data "\
Better to define cursor at the beginning of the method.

http://codereview.appspot.com/1892048/diff/3001/4002#newcode155
sale.py:155: table = TableHandler(self, module_name)
Don't remove cursor on this method.

http://codereview.appspot.com/1892048/diff/3001/4002#newcode168
sale.py:168: table = TableHandler(self, module_name)
Don't remove cursor on this method.

http://codereview.appspot.com/1892048/diff/3001/4002#newcode174
sale.py:174: self.payment_term.domain)
Could be one line

http://codereview.appspot.com/1892048/diff/3001/4002#newcode182
sale.py:182: self.warehouse.domain)
Could be one line

http://codereview.appspot.com/1892048/diff/3001/4002#newcode201
sale.py:201: if company is not None:
I think it is better to not override False value to None and just test: if
company:

http://codereview.appspot.com/1892048/diff/3001/4002#newcode208
sale.py:208: if company is not None:
idem

http://codereview.appspot.com/1892048/diff/3001/4002#newcode328
sale.py:328: if vals.get('lines'):
trailing space

http://codereview.appspot.com/1892048/diff/3001/4002#newcode335
sale.py:335: for tax in tax_obj.compute(line.get('taxes', []),
In fact compute should be called with the context from get_tax_context

http://codereview.appspot.com/1892048/diff/3001/4002#newcode346
sale.py:346: taxes[key])
Could be on one line ?

http://codereview.appspot.com/1892048/diff/3001/4002#newcode434
sale.py:434: tax_list = tax_obj.compute(
Generally we use "taxes" as name.

http://codereview.appspot.com/1892048/diff/3001/4002#newcode929
sale.py:929: table = TableHandler(self, module_name)
Missing cursor

http://codereview.appspot.com/1892048/diff/3001/4002#newcode933
sale.py:933: Transaction().cursor.execute('UPDATE "' + self._table + '" ' \
Instantiate cursor at the beginning of the method.

http://codereview.appspot.com/1892048/diff/3001/4002#newcode1015
sale.py:1015: ctx2 = {}
Could be named context now.

http://codereview.appspot.com/1892048/diff/3001/4002#newcode1045
sale.py:1045: with
Transaction().set_context(self._get_context_sale_price(product,
I will put the _get_context_sale_price on one line

http://codereview.appspot.com/1892048/diff/3001/4002#newcode1091
sale.py:1091: with
Transaction().set_context(self._get_context_sale_price(product,
idem

http://codereview.appspot.com/1892048/diff/3001/4002#newcode1384
sale.py:1384: if Transaction().context.get('uom'):
trailing space

http://codereview.appspot.com/1892048/diff/3001/4002#newcode1592
sale.py:1592: Transaction().cursor.execute('SELECT id FROM sale_invoices_rel ' \
The "\" is not necessary

http://codereview.appspot.com/1892048/diff/3001/4002#newcode1653
sale.py:1653: Transaction().cursor.execute("UPDATE ir_model "\
Instantiate cursor at the beginning.

http://codereview.appspot.com/1892048/diff/3001/4002#newcode1664
sale.py:1664: active_id = Transaction().context.get('active_id') or False
No need of "or False"

http://codereview.appspot.com/1892048/diff/3001/4002#newcode1761
sale.py:1761: active_id = Transaction().context.get('active_id') or False
No need of "or False"

[pheller, 2010-08-04 23:05:49]

Other changes corrected.

I do see many unnecessary explicit line continuations ("\") in the code, I've
removed them all in the patchset I will upload now...  

I've also split a line or two that was > 79 cols

http://codereview.appspot.com/1892048/diff/3001/4002
File sale.py (right):

http://codereview.appspot.com/1892048/diff/3001/4002#newcode346
sale.py:346: taxes[key])
On 2010/08/04 21:48:06, ced wrote:
> Could be on one line ?

Not without exceeding 79 cols or renaming variables...

http://codereview.appspot.com/1892048/diff/3001/4002#newcode434
sale.py:434: tax_list = tax_obj.compute(
On 2010/08/04 21:48:06, ced wrote:
> Generally we use "taxes" as name.

Yes, but there seems to be a "taxes" dict already in scope here....

[ced, 2010-08-05 06:11:15]

http://codereview.appspot.com/1892048/diff/9001/10002
File sale.py (right):

http://codereview.appspot.com/1892048/diff/9001/10002#newcode145
sale.py:145: cursor = Transaction().cursor
Move cursor init before comment

http://codereview.appspot.com/1892048/diff/9001/10002#newcode201
sale.py:201: return company_obj.browse(company).currency.id
trailing space

http://codereview.appspot.com/1892048/diff/9001/10002#newcode334
sale.py:334: for tax in tax_obj.compute(line.get('taxes', []),
Context not fixed

http://codereview.appspot.com/1892048/diff/9001/10002#newcode1653
sale.py:1653: cursor = Transaction().cursor
Move cursor init before comment

[pheller, 2010-08-05 15:04:29]

http://codereview.appspot.com/1892048/diff/9001/10002
File sale.py (right):

http://codereview.appspot.com/1892048/diff/9001/10002#newcode334
sale.py:334: for tax in tax_obj.compute(line.get('taxes', []),
On 2010/08/05 06:11:15, ced wrote:
> Context not fixed

So, I now see what you're getting at.  I removed the two lines referencing
"ctx", but made no other changes.  This is because I didn't see anywhere else in
this function where "ctx" was used...  And, it seems that get_tax_context just
returns a dict.

Perhaps I am missing something here?  How is ctx used?

[ced, 2010-08-05 16:43:20]

Ok for me

[ced, 2010-08-05 16:46:16]

http://codereview.appspot.com/1892048/diff/16001/17002
File sale.py (right):

http://codereview.appspot.com/1892048/diff/16001/17002#newcode1108
sale.py:1108: amount = Decimal(str(vals.get('quantity') or '0.0')) *
Can not remove \

[ced, 2010-08-05 17:22:32]

Ok for me