Merge lp:~acsone-openerp/banking-addons/bank-statement-reconcile-70-improve-acc-number-completion into lp:banking-addons/bank-statement-reconcile-70

[Laurent Mignon (Acsone)]

Improve the matching on acc_number by removing the formatting characters when querying the res_partner_bank table

[Pedro Manuel Baeza (pedro.baeza)]

Some things catched on a first sight:

- Use from . import xxx on __init__.py
- 'if len(ids):' could be 'if ids', or better, put 'if not ids:' and put the rest of the code inside, returning only the value at the end, because it makes it more readable.
- Don't use SQL query. You can use also ORM search with like operator.
- 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.

Regards.

review: Needs Fixing (code review)

[Laurent Mignon (Acsone) (lmi)]

> 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.

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

res_partner_bank.py

Missing encoding definition in res_partner_bank.py

ll.49-51 have indentation issues

About SQL querry isn't it an issue of how are saved account number in database?

Maybe we should ensure data is clean instead of doing special querries.

review: Needs Fixing (code review, no test)

[Pedro Manuel Baeza (pedro.baeza)]

This project is now hosted on https://github.com/OCA/bank-statement-reconcile. Please move your proposal there. This guide may help you https://github.com/OCA/maintainers-tools/wiki/How-to-move-a-Merge-Proposal-to-GitHub

[Laurent Mignon (Acsone) (lmi)]

following steps in
https://github.com/OCA/maintainers-tools/wiki/How-to-move-a-Merge-Proposal-to-GitHub
I'm not able to migrate my MP :(

On Wed, Jul 2, 2014 at 1:05 PM, Pedro Manuel Baeza <email address hidden>
wrote:

> This project is now hosted on
> https://github.com/OCA/bank-statement-reconcile. Please move your
> proposal there. This guide may help you
> https://github.com/OCA/maintainers-tools/wiki/How-to-move-a-Merge-Proposal-to-GitHub
> --
>
> https://code.launchpad.net/~acsone-openerp/banking-addons/bank-statement-reconcile-70-improve-acc-number-completion/+merge/202861
> You proposed
> lp:~acsone-openerp/banking-addons/bank-statement-reconcile-70-improve-acc-number-completion
> for merging.
>

--
*Laurent Mignon*
*Senior Software Engineer*

*Tel : [phone]*
*Fax : [phone]*
*Gsm : [phone]*
*Email: <email address hidden> <email address hidden>*

*Acsone SA, Succursale de Luxembourg*
*22, Zone industrielle*
*L-8287 Kehlen, Luxembourg*
*www.acsone.eu <http://www.acsone.eu>*

[Pedro Manuel Baeza (pedro.baeza)]

I have already problems in this type of migration with some MPs, but not with others, but I'm not a git expert. What I have done is to recreate the diff on git base and make the PR as new. Can you do it this way?

Regards.

[Laurent Mignon (Acsone) (lmi)]

It's what I'm doing ;)
Thank you for your reply.

Regards,

lmi

On Mon, Jul 7, 2014 at 12:36 PM, Pedro Manuel Baeza <email address hidden>
wrote:

> I have already problems in this type of migration with some MPs, but not
> with others, but I'm not a git expert. What I have done is to recreate the
> diff on git base and make the PR as new. Can you do it this way?
>
> Regards.
> --
>
> https://code.launchpad.net/~acsone-openerp/banking-addons/bank-statement-reconcile-70-improve-acc-number-completion/+merge/202861
> You proposed
> lp:~acsone-openerp/banking-addons/bank-statement-reconcile-70-improve-acc-number-completion
> for merging.
>

--
*Laurent Mignon*
*Senior Software Engineer*

*Tel : [phone]*
*Fax : [phone]*
*Gsm : [phone]*
*Email: <email address hidden> <email address hidden>*

*Acsone SA, Succursale de Luxembourg*
*22, Zone industrielle*
*L-8287 Kehlen, Luxembourg*
*www.acsone.eu <http://www.acsone.eu>*

[Laurent Mignon (Acsone) (lmi)]

MP resubmitted to github https://github.com/OCA/bank-statement-reconcile/pull/24

review: Disapprove

Unmerged revisions

113. By Laurent Mignon (Acsone) on 2014-04-24

Use from . import xxx on __init__.py
Improve the SQL used to match account number
Filter results from the plain SQL query by using the method search from the ORM

112. By Laurent Mignon (Acsone) on 2014-04-24

[MRG] lp:banking-addons/bank-statement-reconcile-7.0

111. By Laurent Mignon (Acsone) on 2014-01-23

remove obsolete comment

110. By Laurent Mignon (Acsone) on 2014-01-15

Improve the matching on acc_number by removing the formatting characters when querying the res_partner_bank table

109. By Laurent Mignon (Acsone) on 2014-01-15

clean up tests before modifcations

=== modified file 'account_statement_bankaccount_completion/__init__.py'
--- account_statement_bankaccount_completion/__init__.py	2013-08-22 15:09:03 +0000
+++ account_statement_bankaccount_completion/__init__.py	2014-04-24 10:53:38 +0000
@@ -18,4 +18,5 @@
 #    along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
 #
-import statement
+from . import statement
+from . import res_partner_bank
 
=== added file 'account_statement_bankaccount_completion/res_partner_bank.py'
--- account_statement_bankaccount_completion/res_partner_bank.py	1970-01-01 00:00:00 +0000
+++ account_statement_bankaccount_completion/res_partner_bank.py	2014-04-24 10:53:38 +0000
@@ -0,0 +1,49 @@
+#
+# 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.osv.orm import Model
+from openerp.addons.base_iban import base_iban
+
+
+class res_partner_bank(Model):
+    _inherit = 'res.partner.bank'
+
+    def search_by_acc_number(self, cr, uid, acc_number, context=None):
+        '''
+        Try to find the Account Number using a 'like' operator to avoid problems with the input mask
+        used to store the value.
+        '''
+        # first try with an exact match
+        ids = self.search(cr,
+                                  uid,
+                                  [('acc_number', '=', acc_number)],
+                                  context=context)
+        if ids:
+            return ids
+
+        cr.execute("""
+            SELECT
+                id
+            FROM
+                res_partner_bank
+            WHERE
+                regexp_replace(acc_number,'([^[:alnum:]])', '','g')  ilike  regexp_replace(%s,'([^[:alnum:]])', '','g') """,
+            (acc_number,))
+        #apply security constraints by using the orm
+        return self.search(cr, uid, [('id', 'in', [r[0] for r in cr.fetchall()])])
 
=== modified file 'account_statement_bankaccount_completion/statement.py'
--- account_statement_bankaccount_completion/statement.py	2013-09-12 08:36:23 +0000
+++ account_statement_bankaccount_completion/statement.py	2014-04-24 10:53:38 +0000
@@ -60,9 +60,9 @@
         st_obj = self.pool.get('account.bank.statement.line')
         res = {}
         res_bank_obj = self.pool.get('res.partner.bank')
-        ids = res_bank_obj.search(cr,
+        ids = res_bank_obj.search_by_acc_number(cr,
                                   uid,
-                                  [('acc_number', '=', partner_acc_number)],
+                                  partner_acc_number,
                                   context=context)
         if len(ids) > 1:
             raise ErrorTooManyPartner(_('Line named "%s" (Ref:%s) was matched by more than '
 
=== modified file 'account_statement_bankaccount_completion/tests/test_bankaccount_completion.py'
--- account_statement_bankaccount_completion/tests/test_bankaccount_completion.py	2013-09-12 09:05:01 +0000
+++ account_statement_bankaccount_completion/tests/test_bankaccount_completion.py	2014-04-24 10:53:38 +0000
@@ -22,29 +22,28 @@
 from openerp.tests import common
 import time
 
-ACC_NUMBER = "BE38733040385372"
+ACC_NUMBER = " BE38 7330 4038 5372 "
 
 
 class bankaccount_completion(common.TransactionCase):
 
-    def prepare(self):
+    def setUp(self):
+        super(bankaccount_completion, self).setUp()
         self.company_a = self.browse_ref('base.main_company')
         self.profile_obj = self.registry("account.statement.profile")
         self.account_bank_statement_obj = self.registry("account.bank.statement")
         self.account_bank_statement_line_obj = self.registry("account.bank.statement.line")
         self.completion_rule_id = self.ref('account_statement_bankaccount_completion.bank_statement_completion_rule_10')
-        self.journal_id = self.registry("ir.model.data").get_object_reference(self.cr, self. uid, "account", "bank_journal")[1]
+        self.journal_id = self.ref("account.bank_journal")
         self.partner_id = self.ref('base.main_partner')
+        self.account_id = self.ref("account.a_recv")
+
         # Create the profile
-        self.account_id = self.registry("ir.model.data").get_object_reference(self.cr, self.uid, "account", "a_recv")[1]
-        self.journal_id = self.registry("ir.model.data").get_object_reference(self.cr, self. uid, "account", "bank_journal")[1]
         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 the completion rule
-
         # Create a bank statement
         self.statement_id = self.account_bank_statement_obj.create(self.cr, self.uid, {
             "balance_end_real": 0.0,
@@ -55,18 +54,9 @@
 
         })
 
-        # Create bank a statement line
-        self.statement_line_id = self.account_bank_statement_line_obj.create(self.cr, self.uid, {
-            'amount': 1000.0,
-            'name': 'EXT001',
-            'ref': 'My ref',
-            'statement_id': self.statement_id,
-            'partner_acc_number': ACC_NUMBER
-        })
-
         # Add a bank account number to the partner
-        res_bank_obj = self.registry('res.partner.bank')
-        res_bank_obj.create(self.cr, self.uid, {
+        self.res_partner_bank_obj = self.registry('res.partner.bank')
+        self.res_partner_bank_id = self.res_partner_bank_obj.create(self.cr, self.uid, {
                             "state": "bank",
                             "company_id": self.company_a.id,
                             "partner_id": self.partner_id,
@@ -77,15 +67,39 @@
 
     def test_00(self):
         """Test complete partner_id from bank account number
-        
         Test the automatic completion of the partner_id based on the account number associated to the
         statement line
         """
-        self.prepare()
-        statement_line = self.account_bank_statement_line_obj.browse(self.cr, self.uid, self.statement_line_id)
-        # before import, the
-        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, self.statement_line_id)
-        self.assertEquals(self.partner_id, statement_line.partner_id['id'], "Missing expected partner id after completion")
+        for bank_acc_number in [ACC_NUMBER, ACC_NUMBER.replace(" ", ""), ACC_NUMBER.replace(" ", "-")]:
+            # check the completion for well formatted and not well formatted account number
+            self.res_partner_bank_obj.write(self.cr, self.uid, self.res_partner_bank_id, {
+                            "acc_number": bank_acc_number,
+                            })
+            for acc_number in [ACC_NUMBER, ACC_NUMBER.replace(" ", ""), ACC_NUMBER.replace(" ", "-"), " BE38-7330 4038-5372 "]:
+                statement_line_id = self.account_bank_statement_line_obj.create(self.cr, self.uid, {
+                    'amount': 1000.0,
+                    'name': 'EXT001',
+                    'ref': 'My ref',
+                    'statement_id': self.statement_id,
+                    'partner_acc_number': acc_number
+                })
+                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)
+                self.assertEquals(self.partner_id, statement_line.partner_id['id'], "Missing expected partner id after completion")
+
+            statement_line_id = self.account_bank_statement_line_obj.create(self.cr, self.uid, {
+                    'amount': 1000.0,
+                    'name': 'EXT001',
+                    'ref': 'My ref',
+                    'statement_id': self.statement_id,
+                    'partner_acc_number': 'BE38a7330.4038-5372.'
+                })
+            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)
+            self.assertFalse(statement_line.partner_id.id)