Merge lp:~acsone-openerp/banking-addons/7.0-bug-1280814-base_completion into lp:banking-addons/bank-statement-reconcile-70

[Laurent Mignon (Acsone)]

[FIX] completion on partner name in the statement line label (lp:1280814)

[lp:~acsone-openerp/banking-addons/7.0-bug-1280814-base_completion]

121. By Laurent Mignon (Acsone) on 2014-02-16

a little query optimization

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

Thanks for the fix which LGTM. This code really needs some automated test though.

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

Hmm not sure if your fix is really ok, though:

regexp_matches(string text, pattern text [, flags text])

it seems to me that you have either inverted the args to that function, or you end up using re.escape on the wrong parameter.

Automated test definitely needed.

review: Needs Fixing (code review, no test)

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

> Hmm not sure if your fix is really ok, though:
>
> regexp_matches(string text, pattern text [, flags text])
>
> it seems to me that you have either inverted the args to that function, or you
> end up using re.escape on the wrong parameter.
>
> Automated test definitely needed.

my bad, the automated test is there.

[Laurent Mignon (Acsone) (lmi)]

> my bad, the automated test is there.
Thanks for the review and, yes, automated tests have been added by the MP https://code.launchpad.net/~camptocamp/banking-addons/bank-statement-reconcile-7.0-add-tests-no-split-lep/+merge/199835
Do you maintain your 'Needs Fixing'?
Regards,

lmi

[Leonardo Pistone (lepistone)]

Thanks for your work, Laurent.

@alexandre: we're green! can you imagine a new case we can test to find a problem in Laurent's code?

The fix for the typo porfile_id -> profile_id seems correct, as I can't find the wrong spelling anywhere in the code.

The fact that we had not realised that worries be a bit, though.

review: Needs Information (code review, test)

[Laurent Mignon (Acsone) (lmi)]

Thanks for the review Leonardo

> The fact that we had not realised that worries be a bit, though.

The type only appeared once the search has been fixed and has returned a matching partner. I ask me if this completion rule has been functional in the past.

Regards,

lmi

[Leonardo Pistone (lepistone)]

Thanks Laurent.

I lift my objection on the typo then, and I leave Alexandre with his remark on the regexp.

review: Approve (code review)

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

>
> > my bad, the automated test is there.
> Thanks for the review and, yes, automated tests have been added by the MP
> https://code.launchpad.net/~camptocamp/banking-addons/bank-statement-
> reconcile-7.0-add-tests-no-split-lep/+merge/199835
> Do you maintain your 'Needs Fixing'?
> Regards,
>
> lmi

my understanding of regexp_matches is that arg 1 is the string we want to check, and arg 2 is the regular expression against which we to the matching (http://www.postgresql.org/docs/9.1/static/functions-matching.html)

In the code you propose to merge, you pass a regular expression ('.*' + re.escape(str) + '.*') as the first argument, and a string from the db (which may contain unescaped regular expression special characters such as .) as the regexp value. So this needs to be fixed : at the very least the convertsion of st_line['name'] to a regexp should be removed. And I'm interested in a test in which a partner has name "jh..doe" and a statement line has value 'jheudoe'.

[Laurent Mignon (Acsone) (lmi)]

Hi Alexandre,

I'll review the code according to your comments.

lmi

[lp:~acsone-openerp/banking-addons/7.0-bug-1280814-base_completion]

122. By Laurent Mignon (Acsone) on 2014-02-17

[FIX] escape res_partner.name column as second arg of regexp_matches and keep the first one as unescaped. Add a lot of tests for completion based on the res_partner name

[Laurent Mignon (Acsone) (lmi)]

> my understanding of regexp_matches is that arg 1 is the string we want to
> check, and arg 2 is the regular expression against which we to the matching
> (http://www.postgresql.org/docs/9.1/static/functions-matching.html)
>
> In the code you propose to merge, you pass a regular expression ('.*' +
> re.escape(str) + '.*') as the first argument, and a string from the db (which
> may contain unescaped regular expression special characters such as .) as the
> regexp value. So this needs to be fixed : at the very least the convertsion of
> st_line['name'] to a regexp should be removed. And I'm interested in a test in
> which a partner has name "jh..doe" and a statement line has value 'jheudoe'.

Hi Alexandre,

The code has been modified according to your review.

lmi

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

Thanks for the code update.

LGTM

review: Approve (code review)

[Pedro Manuel Baeza (pedro.baeza)]

Thanks for the fix!

review: Approve (code review)

=== modified file 'account_statement_base_completion/__init__.py'
--- account_statement_base_completion/__init__.py	2012-12-20 13:37:01 +0000
+++ account_statement_base_completion/__init__.py	2014-02-17 17:24:16 +0000
@@ -19,5 +19,5 @@
 #
 ##############################################################################
 
-import statement
-import partner
+from . import partner
+from . import statement

=== modified file 'account_statement_base_completion/statement.py'
--- account_statement_base_completion/statement.py	2014-02-02 10:58:40 +0000
+++ account_statement_base_completion/statement.py	2014-02-17 17:24:16 +0000
@@ -319,9 +319,12 @@
         if not context['partner_memoizer']:
             return res
         st_obj = self.pool.get('account.bank.statement.line')
-        sql = "SELECT id FROM res_partner WHERE name ~* %s and id in %s"
-        pattern = ".*%s.*" % re.escape(st_line['name'])
-        cr.execute(sql, (pattern, context['partner_memoizer']))
+        # regexp_replace(name,'([^a-zA-Z0-9 -])', '\\\1', 'g'), 'i') escape the column name to avoid false positive. (ex 'jho..doe' -> 'joh\.\.doe'
+        sql = """SELECT id FROM  (
+                        SELECT id, regexp_matches(%s, regexp_replace(name,'([^[:alpha:]0-9 -])', %s, 'g'), 'i') AS name_match FROM res_partner
+                            WHERE id IN %s) AS res_patner_matcher
+                    WHERE name_match IS NOT NULL"""
+        cr.execute(sql, (st_line['name'], r"\\\1", context['partner_memoizer']))
         result = cr.fetchall()
         if not result:
             return res
@@ -332,7 +335,7 @@
         res['partner_id'] = result[0][0]
         st_vals = st_obj.get_values_for_line(cr,
                                              uid,
-                                             profile_id=st_line['porfile_id'],
+                                             profile_id=st_line['profile_id'],
                                              master_account_id=st_line['master_account_id'],
                                              partner_id=res['partner_id'],
                                              line_type=False,

=== added directory 'account_statement_base_completion/tests'
=== added file 'account_statement_base_completion/tests/__init__.py'
--- account_statement_base_completion/tests/__init__.py	1970-01-01 00:00:00 +0000
+++ account_statement_base_completion/tests/__init__.py	2014-02-17 17:24:16 +0000
@@ -0,0 +1,27 @@
+# -*- coding: utf-8 -*-
+#
+#
+# Authors: Laurent Mignon
+# Copyright (c) 2014 Acsone SA/NV (http://www.acsone.eu)
+# All Rights Reserved
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as
+# published by the Free Software Foundation, either version 3 of the
+# License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+#
+
+from . import test_base_completion
+
+checks = [
+    test_base_completion
+]

=== added file 'account_statement_base_completion/tests/test_base_completion.py'
--- account_statement_base_completion/tests/test_base_completion.py	1970-01-01 00:00:00 +0000
+++ account_statement_base_completion/tests/test_base_completion.py	2014-02-17 17:24:16 +0000
@@ -0,0 +1,95 @@
+# -*- coding: utf-8 -*-
+#
+#
+# Authors: Laurent Mignon
+# Copyright (c) 2014 Acsone SA/NV (http://www.acsone.eu)
+# All Rights Reserved
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU Affero General Public License as
+# published by the Free Software Foundation, either version 3 of the
+# License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU Affero General Public License for more details.
+#
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+#
+from openerp.tests import common
+import time
+from collections import namedtuple
+
+name_completion_case = namedtuple("name_completion_case", ["partner_name", "line_label", "should_match"])
+NAMES_COMPLETION_CASES = [
+         name_completion_case("Acsone", "Line for Acsone SA", True),
+         name_completion_case("Acsone", "Line for Acsone", True),
+         name_completion_case("Acsone", "Acsone for line", True),
+         name_completion_case("acsone", "Acsone for line", True),
+         name_completion_case("Acsone SA", "Line for Acsone SA test", True),
+         name_completion_case("Ac..ne", "Acsone for line", False),
+         name_completion_case("é@|r{}", "Acsone é@|r{} for line", True),
+         name_completion_case("Acsone", "A..one for line", False),
+         name_completion_case("A.one SA", "A.one SA for line", True),
+         name_completion_case("Acsone SA", "Line for Acsone ([^a-zA-Z0-9 -]) SA test", False),
+         name_completion_case("Acsone ([^a-zA-Z0-9 -]) SA", "Line for Acsone ([^a-zA-Z0-9 -]) SA test", True),
+        ]
+
+
+class base_completion(common.TransactionCase):
+
+    def setUp(self):
+        super(base_completion, self).setUp()
+        self.company_a = self.browse_ref('base.main_company')
+        self.profile_obj = self.registry("account.statement.profile")
+        self.partner_obj = self.registry("res.partner")
+        self.account_bank_statement_obj = self.registry("account.bank.statement")
+        self.account_bank_statement_line_obj = self.registry("account.bank.statement.line")
+        self.journal_id = self.ref("account.bank_journal")
+        self.partner_id = self.ref('base.main_partner')
+        self.account_id = self.ref("account.a_recv")
+        self.partner_id = self.ref("base.res_partner_12")
+
+    def test_name_completion(self):
+        """Test complete partner_id from statement line label
+        Test the automatic completion of the partner_id based if the name of the partner appears in
+        the statement line label
+        """
+        self.completion_rule_id = self.ref('account_statement_base_completion.bank_statement_completion_rule_3')
+        # Create the profile
+        self.profile_id = self.profile_obj.create(self.cr, self.uid, {
+            "name": "TEST",
+            "commission_account_id": self.account_id,
+            "journal_id": self.journal_id,
+            "rule_ids": [(6, 0, [self.completion_rule_id])]})
+        # Create a bank statement
+        self.statement_id = self.account_bank_statement_obj.create(self.cr, self.uid, {
+            "balance_end_real": 0.0,
+            "balance_start": 0.0,
+            "date": time.strftime('%Y-%m-%d'),
+            "journal_id": self.journal_id,
+            "profile_id": self.profile_id
+        })
+
+        for case in NAMES_COMPLETION_CASES:
+            self.partner_obj.write(self.cr, self.uid, self.partner_id, {'name': case.partner_name})
+            statement_line_id = self.account_bank_statement_line_obj.create(self.cr, self.uid, {
+                    'amount': 1000.0,
+                    'name': case.line_label,
+                    'ref': 'My ref',
+                    'statement_id': self.statement_id,
+                })
+            statement_line = self.account_bank_statement_line_obj.browse(self.cr, self.uid, statement_line_id)
+            self.assertFalse(statement_line.partner_id, "Partner_id must be blank before completion")
+            statement_obj = self.account_bank_statement_obj.browse(self.cr, self.uid, self.statement_id)
+            statement_obj.button_auto_completion()
+            statement_line = self.account_bank_statement_line_obj.browse(self.cr, self.uid, statement_line_id)
+            if case.should_match:
+                self.assertEquals(self.partner_id, statement_line.partner_id['id'],
+                                "Missing expected partner id after completion (partner_name: %s, line_name: %s)" % (case.partner_name, case.line_label))
+            else:
+                self.assertNotEquals(self.partner_id, statement_line.partner_id['id'],
+                                     "Partner id should be empty after completion(partner_name: %s, line_name: %s)" % (case.partner_name, case.line_label))