Code review comment for lp:~acsone-openerp/banking-addons/bank-statement-reconcile-70-improve-acc-number-completion

Revision history for this message
Laurent Mignon (Acsone) (lmi) wrote :

> Some things catched on a first sight:
>
> - Use from . import xxx on __init__.py
OK I'll change

> - 'if len(ids):' could be 'if ids', or better, put 'if not ids:' and put the
OK
> rest of the code inside, returning only the value at the end, because it makes
> it more readable.
It's a point of view. I prefer to avoid too much indentation and return as soon as possible.

> - Don't use SQL query. You can use also ORM search with like operator.
The problem is not the 'like' operator, but the field to match "replace(replace(acc_number,' ',''),'-','')" The search method of the orm require a valid field name but in my case it's an expression. One thing can be done to avoid the bypass of the orm in this case (for security, etc), is to add a second search using the orm with an expression [('ids', 'in', ids] where ids is the result of the first search.

> - Complete tests with calls to this method with more cases: account number
> without spaces that must be right, account number that must be wrong, so that
> we can test false positives.
>
'account number without spaces' is already tested. I'll add a test for wrong account number.
> Regards.

« Back to merge proposal