Merge lp:~camptocamp/banking-addons/improve_lookup into lp:banking-addons/bank-statement-reconcile-70

[Nicolas Bessi - Camptocamp]

Fixes performance trouble when using bank_statement_label based completion rules by using memoizer pattern.

Add lines in context to be able to acces them in completion rules. It is not mandatory as we can do line.satement_id.line_ids but it is more efficient.

Some minor cleanup

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

My main concern is the generation of an invalid regular expression on lines 30-31 : what if the partner.bank_statement_label contains "special" characters such as .[]()^$+*? this could lead to a crash in the query.

Unless we are really really sure this is not possible, there should be some escaping performed on this string before converting it to a regex. I think there is a re.escape function available in Python to do just this.

line 37-38: useless, please remove

line 23: is context['label_memoizer'] used outside this function? If not, this could be simply a local variable.

line 101: get is a method -> use () instead of [] (or if you're sure the key is in there, just [] without get)

review: Needs Fixing (code review, no test)

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

> My main concern is the generation of an invalid regular expression on lines
> 30-31 : what if the partner.bank_statement_label contains "special" characters
> such as .[]()^$+*? this could lead to a crash in the query.
>
> Unless we are really really sure this is not possible, there should be some
> escaping performed on this string before converting it to a regex. I think
> there is a re.escape function available in Python to do just this.
>
> line 37-38: useless, please remove
>
> line 23: is context['label_memoizer'] used outside this function? If not,
> this could be simply a local variable.
>
> line 101: get is a method -> use () instead of [] (or if you're sure the key
> is in there, just [] without get)

and l21: s/Follwing/Following/

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Hello,

Thank for your comments

I think I will add constraints on the bank_label_field this will be more explicit to the end user. What do you think.

line 23: The memoizer is passed trough each line of the statement it is not locally scoped as the function is called for each line of the statement. That's why the previous implementation was innefficient.

line 101. Tho.. finger crossed, I wanted to remove the get usage. That why we made code review ;)

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

After a discution with Frederic this pattern are widely used. So I think I'll use ilike or is similar to expression

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Ok we have a misunderstanding with Fréderic about the specifications.
re.escape will do the trick.

Regards

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Hello,
Proposed fixes added

[Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c)]

LGTM

On a totally minor side, I'd prefer "import re" and using "re.escape" the code; The reason being that there are several "escape" functions in the stdlib (for xml, cgi, shell command lines, re...) and it is easier for the reader when coming on the code to understand the context with a namespaced call.

review: Approve

[lp:~camptocamp/banking-addons/improve_lookup]

90. By Nicolas Bessi - Camptocamp on 2013-03-22

[IMP] import readability

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Improve import

=== modified file 'account_statement_base_completion/statement.py'
--- account_statement_base_completion/statement.py	2013-03-01 13:33:03 +0000
+++ account_statement_base_completion/statement.py	2013-03-22 08:33:26 +0000
@@ -18,6 +18,9 @@
 #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 ##############################################################################
+from collections import defaultdict
+import re
+
 from tools.translate import _
 from openerp.osv.orm import Model, fields
 from openerp.tools import DEFAULT_SERVER_DATETIME_FORMAT
@@ -269,35 +272,44 @@
             """
         partner_obj = self.pool.get('res.partner')
         st_obj = self.pool.get('account.bank.statement.line')
+        res = {}
+        # As we have to iterate on each partner for each line,
+        # we memoize the pair to avoid
+        # to redo computation for each line.
+        # Following code can be done by a single SQL query
+        # but this option is not really maintanable
+        if not context.get('label_memoizer'):
+            context['label_memoizer'] = defaultdict(list)
+            partner_ids = partner_obj.search(cr,
+                                             uid,
+                                             [('bank_statement_label', '!=', False)])
+            line_ids = tuple(x.id for x in context.get('line_ids', []))
+            for partner in partner_obj.browse(cr, uid, partner_ids, context=context):
+                vals = '|'.join(re.escape(x.strip()) for x in partner.bank_statement_label.split(';'))
+                or_regex = ".*%s*." % vals
+                sql = ("SELECT id from account_bank_statement_line"
+                       " WHERE id in %s"
+                       " AND name ~* %s")
+                cr.execute(sql, (line_ids, or_regex))
+                pairs = cr.fetchall()
+                for pair in pairs:
+                    context['label_memoizer'][pair[0]].append(partner)
         st_line = st_obj.browse(cr, uid, line_id, context=context)
-        res = {}
-        compt = 0
-        if st_line:
-            ids = partner_obj.search(
-                    cr,
-                    uid,
-                    [('bank_statement_label', '!=', False)],
-                    context=context)
-            for partner in partner_obj.browse(cr, uid, ids, context=context):
-                for partner_label in partner.bank_statement_label.split(';'):
-                    if partner_label in st_line.label:
-                        compt += 1
-                        res['partner_id'] = partner.id
-                        if compt > 1:
-                            raise ErrorTooManyPartner(
-                                    _('Line named "%s" (Ref:%s) was matched by '
-                                      'more than one partner.') %
-                                    (st_line.name, st_line.ref))
-            if res:
-                st_vals = st_obj.get_values_for_line(
-                        cr,
-                        uid,
-                        profile_id=st_line.statement_id.profile_id.id,
-                        partner_id=res.get('partner_id', False),
-                        line_type=st_line.type,
-                        amount=st_line.amount,
-                        context=context)
-                res.update(st_vals)
+        if st_line and st_line.id in context['label_memoizer']:
+            found_partner = context['label_memoizer'][st_line.id]
+            if len(found_partner) > 1:
+                raise ErrorTooManyPartner(_('Line named "%s" (Ref:%s) was matched by '
+                                            'more than one partner.') %
+                                          (st_line.name, st_line.ref))
+            res['partner_id'] = found_partner[0].id
+            st_vals = st_obj.get_values_for_line(cr,
+                                                 uid,
+                                                 profile_id=st_line.statement_id.profile_id.id,
+                                                 partner_id=found_partner[0].id,
+                                                 line_type=st_line.type,
+                                                 amount=st_line.amount,
+                                                 context=context)
+            res.update(st_vals)
         return res
 
     def get_from_label_and_partner_name(self, cr, uid, line_id, context=None):
@@ -322,24 +334,22 @@
         st_line = st_obj.browse(cr, uid, line_id, context=context)
         if st_line:
             sql = "SELECT id FROM res_partner WHERE name ~* %s"
-            pattern = ".*%s.*" % st_line.label
+            pattern = ".*%s.*" % re.escape(st_line.label)
             cr.execute(sql, (pattern,))
             result = cr.fetchall()
             if not result:
                 return res
             if len(result) > 1:
-                raise ErrorTooManyPartner(
-                        _('Line named "%s" (Ref:%s) was matched by more '
-                          'than one partner.') %
-                        (st_line.name, st_line.ref))
-            for id in result[0]:
-                res['partner_id'] = id
+                raise ErrorTooManyPartner(_('Line named "%s" (Ref:%s) was matched by more '
+                                            'than one partner.') %
+                                          (st_line.name, st_line.ref))
+            res['partner_id'] = result[0][0] if result else False
             if res:
                 st_vals = st_obj.get_values_for_line(
                         cr,
                         uid,
                         profile_id=st_line.statement_id.profile_id.id,
-                        partner_id=res.get('partner_id', False),
+                        partner_id=res['partner_id'],
                         line_type=st_line.type,
                         amount=st_line.amount,
                         context=context)
@@ -475,6 +485,7 @@
         for stat in self.browse(cr, uid, ids, context=context):
             msg_lines = []
             ctx = context.copy()
+            ctx['line_ids'] = stat.line_ids
             for line in stat.line_ids:
                 res = {}
                 try: