Merge lp:~therp-nl/banking-addons/ba61-multicompany_safe_partner_search into lp:banking-addons/6.1

Proposed by Stefan Rijnhart (Opener)
Status: Merged
Merged at revision: 161
Proposed branch: lp:~therp-nl/banking-addons/ba61-multicompany_safe_partner_search
Merge into: lp:banking-addons/6.1
Diff against target: 133 lines (+31/-39)
2 files modified
account_banking/ (+1/-2)
account_banking/wizard/ (+30/-37)
To merge this branch: bzr merge lp:~therp-nl/banking-addons/ba61-multicompany_safe_partner_search
Reviewer Review Type Date Requested Status
Alexandre Fayolle - camptocamp Approve
Guewen Baconnier @ Camptocamp code review, no test Approve
Ronald Portier (Therp) Approve
Review via email:

Description of the change

This branch fixes partner search in a multicompany database. As there are no multicompany rules on addresses, the module can trigger the 'Access prohibited by access rules' error when trying to read partners through their addresses.

- match by partner name is slightly improved by disregarding very short partner names.
- name of the database cursor has been standardized to 'cr' in this method.
- already disabled code for searching by address components has been removed
- renamed variable name 'filter' to 'filter_' as 'filter' is a keyword (adding trailing underscore as per pep8)
- context propagation and PEP8
- removed unused imports

To post a comment you must log in.
Revision history for this message
Ronald Portier (Therp) (rportier1962) wrote :

Why not replace filter with criteria (for example)? IMHO the trailing underscore is a pain on the eyes.

More importantly, I get the impression that the way the code is changed will result in a great multitude of false positives. Especially when searching for very common names.

Maybe I see it completely wrong, but it seems that on l.26 you empty all criteria, and then on l.46 you are going to search for ALL partners with an address in the same country. Later, on lines 59-62 you filter out all partners with a name (that should be longer then 3 positions) that is not part of the name passed.

I think the search for city and postal code are not disabled in the original code, and should not be in the adapted code.

My apologies in advance if I somehow misread your changes.

One thing more: there should no print statement left in the final code.

review: Needs Fixing
161. By Stefan Rijnhart (Opener)

[FIX] Reinstate search by zip and city
[RFR] Remove debug statement

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

Well my apologies for uploading code before my second cup of coffee ;-)

You are right on all points. About the underscore, it's what PEP8 says but I don't want to hurt anyone's eyes. Should all be fixed now.

Revision history for this message
Ronald Portier (Therp) (rportier1962) wrote :

Approve LGTM

One other idea;
If multiple names are found, maybe better to take the longest, instead of the first?

Lets assume we have the following partner names:

The function is passed the name Smithsons Ltd.

None of the partner names is ilike Smithsons Ltd., so we have to do a brute force search.
This will eliminate Smithsonian (not in Smithsons Ltd.)

Then Smithsons would seem to be the better match.

review: Approve
Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

I notice no context is provided and therefore none is propagated.

Regarding the test 'if (not country_code) or not country_id' :

* would "if not (country_code and country_id)" be more readable? (not sure about this, and see 2nd point below)

* from the code higher in the function, we get there if a country_code was provided but the corresponding country was not found. My understanding of the execution flow leads me to think that the first part of the test is to guard against a NameError because country_id is only defined if country_code was defined. In that case, I recommend setting country_id to False at the beginning of the function, which allows to rewrite the test as "if not country_id"

review: Needs Information (code review, no test)
162. By Stefan Rijnhart (Opener)

[RFR] Refactor double check on country_id and country_code
[IMP] Select longest matching partner name if multiple matches
[IMP] Propagate context in get_or_create_partner

163. By Stefan Rijnhart (Opener)

[FIX] Lingering parenthesis

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

@Ronald Thanks for the suggestion, I have implemented 'longest matching partner name wins'

@Alexandre Thanks, I changed the logic around and took the opportunity to refactor this part of the code with respect to context propagation and line length. Lots of legacy issues in the rest of this module left but we'll get there piece by piece.

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :


review: Approve (code review, no test)
Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

"we'll get there piece by piece" -> yes, that's the idea. I only came across this in this review because it showed up in the context diff... We can't really afford a code review of the whole code base, but when improvements opportunities show up, we should take advantage of them.

Thanks for the refactoring.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'account_banking/'
2--- account_banking/ 2012-12-18 14:42:08 +0000
3+++ account_banking/ 2013-02-07 09:58:26 +0000
4@@ -1314,8 +1314,7 @@
5 transaction.remote_owner_address,
6 transaction.remote_owner_postalcode,
7 transaction.remote_owner_city,
8- country_code, results['log']
9- )
10+ country_code, results['log'], context=context)
11 if transaction.remote_account:
12 partner_bank_id = create_bank_account(
13 self.pool, cr, uid, partner_id,
15=== modified file 'account_banking/wizard/'
16--- account_banking/wizard/ 2012-12-18 14:41:15 +0000
17+++ account_banking/wizard/ 2013-02-07 09:58:26 +0000
18@@ -19,14 +19,11 @@
19 #
20 ##############################################################################
22-import sys
23 import datetime
24-import re
25 from tools.translate import _
26 from account_banking.parsers import convert
27 from account_banking import sepa
28 from account_banking.struct import struct
29-import unicodedata
31 __all__ = [
32 'get_period',
33@@ -121,71 +118,67 @@
34 except KeyError:
35 return False
37-def get_or_create_partner(pool, cursor, uid, name, address, postal_code, city,
38- country_code, log):
39+def get_or_create_partner(pool, cr, uid, name, address, postal_code, city,
40+ country_code, log, context=None):
41 '''
42 Get or create the partner belonging to the account holders name <name>
44 If multiple partners are found with the same name, select the first and
45 add a warning to the import log.
47+ TODO: revive the search by lines from the address argument
48 '''
49 partner_obj = pool.get('res.partner')
50- partner_ids =, uid, [('name', 'ilike', name)])
51+ partner_ids =, uid, [('name', 'ilike', name)],
52+ context=context)
53+ country_id = False
54 if not partner_ids:
55 # Try brute search on address and then match reverse
56- address_obj = pool.get('res.partner.address')
57- filter = [('partner_id', '<>', None)]
58+ criteria = []
59 if country_code:
60 country_obj = pool.get('')
61 country_ids =
62- cursor, uid, [('code','=',country_code.upper())]
63- )
64+ cr, uid, [('code', '=', country_code.upper())],
65+ context=context)
66 country_id = country_ids and country_ids[0] or False
67- filter.append(('country_id', '=', country_id))
68- # disable for now. Apparently, address is an array of lines.
69- if address and False:
70- if len(address) >= 1:
71- filter.append(('street', 'ilike', address[0]))
72- if len(address) > 1:
73- filter.append(('street2', 'ilike', address[1]))
74+ criteria.append(('address.country_id', '=', country_id))
75 if city:
76- filter.append(('city', 'ilike', city))
77+ criteria.append(('', 'ilike', city))
78 if postal_code:
79- filter.append(('zip', 'ilike', postal_code))
80- address_ids =, uid, filter)
81+ criteria.append(('', 'ilike', postal_code))
82+ partner_search_ids =
83+ cr, uid, criteria, context=context)
84 key = name.lower()
86- # Make sure to get a unique list
87- partner_ids = list(set([
88- for x in address_obj.browse(cursor, uid, address_ids)
89- # Beware for dangling addresses
90- if _has_attr(x.partner_id, 'name') and
91- in key
92- ]))
93+ partners = []
94+ for partner in
95+ cr, uid, partner_search_ids, ['name'], context=context):
96+ if (len(partner['name']) > 3 and partner['name'].lower() in key):
97+ partners.append(partner)
98+ partners.sort(key=lambda x: len(x['name']), reverse=True)
99+ partner_ids = [x['id'] for x in partners]
100 if not partner_ids:
101- if (not country_code) or not country_id:
102- user = pool.get('res.user').browse(cursor, uid, uid)
103+ if not country_id:
104+ user = pool.get('res.user').browse(cr, uid, uid, context=context)
105 country_id = (
106 and
107 or
108 False
109 )
110- partner_id = partner_obj.create(cursor, uid, dict(
111- name=name, active=True, comment='Generated from Bank Statements Import',
112+ partner_id = partner_obj.create(cr, uid, dict(
113+ name=name, active=True,
114+ comment='Generated from Bank Statements Import',
115 address=[(0,0,{
116 'street': address and address[0] or '',
117 'street2': len(address) > 1 and address[1] or '',
118 'city': city,
119 'zip': postal_code or '',
120 'country_id': country_id,
121- })],
122- ))
123+ })]), context=context)
124 else:
125 if len(partner_ids) > 1:
126 log.append(
127- _('More than one possible match found for partner with name %(name)s')
128- % {'name': name}
129- )
130+ _('More than one possible match found for partner with '
131+ 'name %(name)s') % {'name': name})
132 partner_id = partner_ids[0]
133 return partner_id


People subscribed via source and target branches