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

Proposed by Laurent Mignon (Acsone)
Status: Needs review
Proposed branch: lp:~acsone-openerp/banking-addons/bank-statement-reconcile-70-improve-acc-number-completion
Merge into: lp:banking-addons/bank-statement-reconcile-70
Diff against target: 190 lines (+94/-30)
4 files modified
account_statement_bankaccount_completion/__init__.py (+2/-1)
account_statement_bankaccount_completion/res_partner_bank.py (+49/-0)
account_statement_bankaccount_completion/statement.py (+2/-2)
account_statement_bankaccount_completion/tests/test_bankaccount_completion.py (+41/-27)
To merge this branch: bzr merge lp:~acsone-openerp/banking-addons/bank-statement-reconcile-70-improve-acc-number-completion
Reviewer Review Type Date Requested Status
Laurent Mignon (Acsone) (community) Disapprove
Yannick Vaucher @ Camptocamp code review, no test Needs Fixing
Pedro Manuel Baeza code review Needs Fixing
Review via email: mp+202861@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Pedro Manuel Baeza (pedro.baeza) wrote :

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

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

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)
Revision history for this message
Pedro Manuel Baeza (pedro.baeza) 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

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

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 : +352 20 21 10 20 32*
*Fax : +352 20 21 10 21*
*Gsm : +352 691 506 009*
*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>*

Revision history for this message
Pedro Manuel Baeza (pedro.baeza) 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.

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

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 : +352 20 21 10 20 32*
*Fax : +352 20 21 10 21*
*Gsm : +352 691 506 009*
*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>*

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

Unmerged revisions

113. By Laurent Mignon (Acsone)

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)

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

111. By Laurent Mignon (Acsone)

remove obsolete comment

110. By Laurent Mignon (Acsone)

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

109. By Laurent Mignon (Acsone)

clean up tests before modifcations

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'account_statement_bankaccount_completion/__init__.py'
2--- account_statement_bankaccount_completion/__init__.py 2013-08-22 15:09:03 +0000
3+++ account_statement_bankaccount_completion/__init__.py 2014-04-24 10:53:38 +0000
4@@ -18,4 +18,5 @@
5 # along with this program. If not, see <http://www.gnu.org/licenses/>.
6 #
7 #
8-import statement
9+from . import statement
10+from . import res_partner_bank
11
12=== added file 'account_statement_bankaccount_completion/res_partner_bank.py'
13--- account_statement_bankaccount_completion/res_partner_bank.py 1970-01-01 00:00:00 +0000
14+++ account_statement_bankaccount_completion/res_partner_bank.py 2014-04-24 10:53:38 +0000
15@@ -0,0 +1,49 @@
16+#
17+# Authors: Laurent Mignon
18+# Copyright (c) 2014 Acsone SA/NV (http://www.acsone.eu)
19+# All Rights Reserved
20+#
21+# This program is free software: you can redistribute it and/or modify
22+# it under the terms of the GNU Affero General Public License as
23+# published by the Free Software Foundation, either version 3 of the
24+# License, or (at your option) any later version.
25+#
26+# This program is distributed in the hope that it will be useful,
27+# but WITHOUT ANY WARRANTY; without even the implied warranty of
28+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
29+# GNU Affero General Public License for more details.
30+#
31+# You should have received a copy of the GNU Affero General Public License
32+# along with this program. If not, see <http://www.gnu.org/licenses/>.
33+#
34+#
35+from openerp.osv.orm import Model
36+from openerp.addons.base_iban import base_iban
37+
38+
39+class res_partner_bank(Model):
40+ _inherit = 'res.partner.bank'
41+
42+ def search_by_acc_number(self, cr, uid, acc_number, context=None):
43+ '''
44+ Try to find the Account Number using a 'like' operator to avoid problems with the input mask
45+ used to store the value.
46+ '''
47+ # first try with an exact match
48+ ids = self.search(cr,
49+ uid,
50+ [('acc_number', '=', acc_number)],
51+ context=context)
52+ if ids:
53+ return ids
54+
55+ cr.execute("""
56+ SELECT
57+ id
58+ FROM
59+ res_partner_bank
60+ WHERE
61+ regexp_replace(acc_number,'([^[:alnum:]])', '','g') ilike regexp_replace(%s,'([^[:alnum:]])', '','g') """,
62+ (acc_number,))
63+ #apply security constraints by using the orm
64+ return self.search(cr, uid, [('id', 'in', [r[0] for r in cr.fetchall()])])
65
66=== modified file 'account_statement_bankaccount_completion/statement.py'
67--- account_statement_bankaccount_completion/statement.py 2013-09-12 08:36:23 +0000
68+++ account_statement_bankaccount_completion/statement.py 2014-04-24 10:53:38 +0000
69@@ -60,9 +60,9 @@
70 st_obj = self.pool.get('account.bank.statement.line')
71 res = {}
72 res_bank_obj = self.pool.get('res.partner.bank')
73- ids = res_bank_obj.search(cr,
74+ ids = res_bank_obj.search_by_acc_number(cr,
75 uid,
76- [('acc_number', '=', partner_acc_number)],
77+ partner_acc_number,
78 context=context)
79 if len(ids) > 1:
80 raise ErrorTooManyPartner(_('Line named "%s" (Ref:%s) was matched by more than '
81
82=== modified file 'account_statement_bankaccount_completion/tests/test_bankaccount_completion.py'
83--- account_statement_bankaccount_completion/tests/test_bankaccount_completion.py 2013-09-12 09:05:01 +0000
84+++ account_statement_bankaccount_completion/tests/test_bankaccount_completion.py 2014-04-24 10:53:38 +0000
85@@ -22,29 +22,28 @@
86 from openerp.tests import common
87 import time
88
89-ACC_NUMBER = "BE38733040385372"
90+ACC_NUMBER = " BE38 7330 4038 5372 "
91
92
93 class bankaccount_completion(common.TransactionCase):
94
95- def prepare(self):
96+ def setUp(self):
97+ super(bankaccount_completion, self).setUp()
98 self.company_a = self.browse_ref('base.main_company')
99 self.profile_obj = self.registry("account.statement.profile")
100 self.account_bank_statement_obj = self.registry("account.bank.statement")
101 self.account_bank_statement_line_obj = self.registry("account.bank.statement.line")
102 self.completion_rule_id = self.ref('account_statement_bankaccount_completion.bank_statement_completion_rule_10')
103- self.journal_id = self.registry("ir.model.data").get_object_reference(self.cr, self. uid, "account", "bank_journal")[1]
104+ self.journal_id = self.ref("account.bank_journal")
105 self.partner_id = self.ref('base.main_partner')
106+ self.account_id = self.ref("account.a_recv")
107+
108 # Create the profile
109- self.account_id = self.registry("ir.model.data").get_object_reference(self.cr, self.uid, "account", "a_recv")[1]
110- self.journal_id = self.registry("ir.model.data").get_object_reference(self.cr, self. uid, "account", "bank_journal")[1]
111 self.profile_id = self.profile_obj.create(self.cr, self.uid, {
112 "name": "TEST",
113 "commission_account_id": self.account_id,
114 "journal_id": self.journal_id,
115 "rule_ids": [(6, 0, [self.completion_rule_id])]})
116- # Create the completion rule
117-
118 # Create a bank statement
119 self.statement_id = self.account_bank_statement_obj.create(self.cr, self.uid, {
120 "balance_end_real": 0.0,
121@@ -55,18 +54,9 @@
122
123 })
124
125- # Create bank a statement line
126- self.statement_line_id = self.account_bank_statement_line_obj.create(self.cr, self.uid, {
127- 'amount': 1000.0,
128- 'name': 'EXT001',
129- 'ref': 'My ref',
130- 'statement_id': self.statement_id,
131- 'partner_acc_number': ACC_NUMBER
132- })
133-
134 # Add a bank account number to the partner
135- res_bank_obj = self.registry('res.partner.bank')
136- res_bank_obj.create(self.cr, self.uid, {
137+ self.res_partner_bank_obj = self.registry('res.partner.bank')
138+ self.res_partner_bank_id = self.res_partner_bank_obj.create(self.cr, self.uid, {
139 "state": "bank",
140 "company_id": self.company_a.id,
141 "partner_id": self.partner_id,
142@@ -77,15 +67,39 @@
143
144 def test_00(self):
145 """Test complete partner_id from bank account number
146-
147 Test the automatic completion of the partner_id based on the account number associated to the
148 statement line
149 """
150- self.prepare()
151- statement_line = self.account_bank_statement_line_obj.browse(self.cr, self.uid, self.statement_line_id)
152- # before import, the
153- self.assertFalse(statement_line.partner_id, "Partner_id must be blank before completion")
154- statement_obj = self.account_bank_statement_obj.browse(self.cr, self.uid, self.statement_id)
155- statement_obj.button_auto_completion()
156- statement_line = self.account_bank_statement_line_obj.browse(self.cr, self.uid, self.statement_line_id)
157- self.assertEquals(self.partner_id, statement_line.partner_id['id'], "Missing expected partner id after completion")
158+ for bank_acc_number in [ACC_NUMBER, ACC_NUMBER.replace(" ", ""), ACC_NUMBER.replace(" ", "-")]:
159+ # check the completion for well formatted and not well formatted account number
160+ self.res_partner_bank_obj.write(self.cr, self.uid, self.res_partner_bank_id, {
161+ "acc_number": bank_acc_number,
162+ })
163+ for acc_number in [ACC_NUMBER, ACC_NUMBER.replace(" ", ""), ACC_NUMBER.replace(" ", "-"), " BE38-7330 4038-5372 "]:
164+ statement_line_id = self.account_bank_statement_line_obj.create(self.cr, self.uid, {
165+ 'amount': 1000.0,
166+ 'name': 'EXT001',
167+ 'ref': 'My ref',
168+ 'statement_id': self.statement_id,
169+ 'partner_acc_number': acc_number
170+ })
171+ statement_line = self.account_bank_statement_line_obj.browse(self.cr, self.uid, statement_line_id)
172+ self.assertFalse(statement_line.partner_id, "Partner_id must be blank before completion")
173+ statement_obj = self.account_bank_statement_obj.browse(self.cr, self.uid, self.statement_id)
174+ statement_obj.button_auto_completion()
175+ statement_line = self.account_bank_statement_line_obj.browse(self.cr, self.uid, statement_line_id)
176+ self.assertEquals(self.partner_id, statement_line.partner_id['id'], "Missing expected partner id after completion")
177+
178+ statement_line_id = self.account_bank_statement_line_obj.create(self.cr, self.uid, {
179+ 'amount': 1000.0,
180+ 'name': 'EXT001',
181+ 'ref': 'My ref',
182+ 'statement_id': self.statement_id,
183+ 'partner_acc_number': 'BE38a7330.4038-5372.'
184+ })
185+ statement_line = self.account_bank_statement_line_obj.browse(self.cr, self.uid, statement_line_id)
186+ self.assertFalse(statement_line.partner_id, "Partner_id must be blank before completion")
187+ statement_obj = self.account_bank_statement_obj.browse(self.cr, self.uid, self.statement_id)
188+ statement_obj.button_auto_completion()
189+ statement_line = self.account_bank_statement_line_obj.browse(self.cr, self.uid, statement_line_id)
190+ self.assertFalse(statement_line.partner_id.id)

Subscribers

People subscribed via source and target branches