banking-addons-team team mailing list archive

[Alexandre Fayolle - camptocamp <alexandre.fayolle@xxxxxxxxxxxxxx>]

Review: Needs Fixing code review, no test

General: no space before "!" "?" ":" or ";" in english -> fix needed for the error messages

line 148-149: I find this message confusing, since we are not searching by partner. This message is repeated in several places in the code. 

line 418: is there not a security rules bypass here ? Eg in a multicompany context with non shares res.partners, I can imagine the query returning several rows, yet only one partner being availalble to the current company. 

line 424: the else False part will never execute : the function has returned on line 419. 
line 425: this test is always true (the execution of line 424 sets res to an non empty dictionary, which is True)

line 487-492: there is still a little cleanup to be done here: the previous logic was handling several lines in a loop (hence the need for error_stack). The current logic deals with a single line, so error_stack can be removed and the exception raised in the except block. In the same function, I'd change the "return res" statements to "return {}" as res is never changed. 

line 519: use log_line.insert(0, value) to insert a single element at the beginning of the line (I have not seen anything indicating the the RHS arg is multi element list). 

line 598: leftover print statement


line 874: no batch update is performed. Docstring update required :-)

line 952: any reason for doing this in a loop rather than using UPDATE account_bank_statement_line SET sequence = account_bank_statement_line.id + 1 WHERE statement_id in %(list_of_ids)s" ? 





-- 
https://code.launchpad.net/~camptocamp-reviewer/banking-addons/fix-type-and-account-in-bk-st-line/+merge/160591
Your team Banking Addons Team is subscribed to branch lp:banking-addons/bank-statement-reconcile-70.