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

[Stefan Rijnhart (Opener)]

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.

Additionally:
- 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

[Ronald Portier (Therp) (rportier1962)]

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

[lp:~therp-nl/banking-addons/ba61-multicompany_safe_partner_search]

161. By Stefan Rijnhart (Opener) on 2013-02-06

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

[Stefan Rijnhart (Opener) (stefan-opener)]

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.

[Ronald Portier (Therp) (rportier1962)]

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:
Smith
Smithsonian
Smithsons

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

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

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)

[lp:~therp-nl/banking-addons/ba61-multicompany_safe_partner_search]

162. By Stefan Rijnhart (Opener) on 2013-02-07

[RFR] PEP8
[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) on 2013-02-07

[FIX] Lingering parenthesis

[Stefan Rijnhart (Opener) (stefan-opener)]

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

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

LGTM

review: Approve (code review, no test)

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

"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

=== modified file 'account_banking/banking_import_transaction.py'
--- account_banking/banking_import_transaction.py	2012-12-18 14:42:08 +0000
+++ account_banking/banking_import_transaction.py	2013-02-07 09:58:26 +0000
@@ -1314,8 +1314,7 @@
                         transaction.remote_owner_address,
                         transaction.remote_owner_postalcode,
                         transaction.remote_owner_city,
-                        country_code, results['log']
-                        )
+                        country_code, results['log'], context=context)
                     if transaction.remote_account:
                         partner_bank_id = create_bank_account(
                             self.pool, cr, uid, partner_id,
 
=== modified file 'account_banking/wizard/banktools.py'
--- account_banking/wizard/banktools.py	2012-12-18 14:41:15 +0000
+++ account_banking/wizard/banktools.py	2013-02-07 09:58:26 +0000
@@ -19,14 +19,11 @@
 #
 ##############################################################################
 
-import sys
 import datetime
-import re
 from tools.translate import _
 from account_banking.parsers import convert
 from account_banking import sepa
 from account_banking.struct import struct
-import unicodedata
 
 __all__ = [
     'get_period', 
@@ -121,71 +118,67 @@
     except KeyError:
         return False
 
-def get_or_create_partner(pool, cursor, uid, name, address, postal_code, city,
-                          country_code, log):
+def get_or_create_partner(pool, cr, uid, name, address, postal_code, city,
+                          country_code, log, context=None):
     '''
     Get or create the partner belonging to the account holders name <name>
 
     If multiple partners are found with the same name, select the first and
     add a warning to the import log.
+ 
+    TODO: revive the search by lines from the address argument
     '''
     partner_obj = pool.get('res.partner')
-    partner_ids = partner_obj.search(cursor, uid, [('name', 'ilike', name)])
+    partner_ids = partner_obj.search(cr, uid, [('name', 'ilike', name)],
+                                     context=context)
+    country_id = False
     if not partner_ids:
         # Try brute search on address and then match reverse
-        address_obj = pool.get('res.partner.address')
-        filter = [('partner_id', '<>', None)]
+        criteria = []
         if country_code:
             country_obj = pool.get('res.country')
             country_ids = country_obj.search(
-                cursor, uid, [('code','=',country_code.upper())]
-            )
+                cr, uid, [('code', '=', country_code.upper())],
+                context=context)
             country_id = country_ids and country_ids[0] or False
-            filter.append(('country_id', '=', country_id))
-        # disable for now. Apparently, address is an array of lines.
-        if address and False:
-            if len(address) >= 1:
-                filter.append(('street', 'ilike', address[0]))
-            if len(address) > 1:
-                filter.append(('street2', 'ilike', address[1]))
+            criteria.append(('address.country_id', '=', country_id))
         if city:
-            filter.append(('city', 'ilike', city))
+            criteria.append(('address.city', 'ilike', city))
         if postal_code:
-            filter.append(('zip', 'ilike', postal_code))
-        address_ids = address_obj.search(cursor, uid, filter)
+            criteria.append(('address.zip', 'ilike', postal_code))
+        partner_search_ids = partner_obj.search(
+            cr, uid, criteria, context=context)
         key = name.lower()
-
-        # Make sure to get a unique list
-        partner_ids = list(set([x.partner_id.id
-                       for x in address_obj.browse(cursor, uid, address_ids)
-                       # Beware for dangling addresses
-                       if _has_attr(x.partner_id, 'name') and
-                          x.partner_id.name.lower() in key
-                      ]))
+        partners = []
+        for partner in partner_obj.read(
+            cr, uid, partner_search_ids, ['name'], context=context):
+            if (len(partner['name']) > 3 and partner['name'].lower() in key):
+                partners.append(partner)
+        partners.sort(key=lambda x: len(x['name']), reverse=True)
+        partner_ids = [x['id'] for x in partners]
     if not partner_ids:
-        if (not country_code) or not country_id:
-            user = pool.get('res.user').browse(cursor, uid, uid)
+        if not country_id:
+            user = pool.get('res.user').browse(cr, uid, uid, context=context)
             country_id = (
                 user.company_id.partner_id.country and 
                 user.company_id.partner_id.country.id or
                 False
             )
-        partner_id = partner_obj.create(cursor, uid, dict(
-            name=name, active=True, comment='Generated from Bank Statements Import',
+        partner_id = partner_obj.create(cr, uid, dict(
+            name=name, active=True,
+            comment='Generated from Bank Statements Import',
             address=[(0,0,{
                 'street': address and address[0] or '',
                 'street2': len(address) > 1 and address[1] or '',
                 'city': city,
                 'zip': postal_code or '',
                 'country_id': country_id,
-            })],
-        ))
+            })]), context=context)
     else:
         if len(partner_ids) > 1:
             log.append(
-                _('More than one possible match found for partner with name %(name)s')
-                % {'name': name}
-                )
+                _('More than one possible match found for partner with '
+                  'name %(name)s') % {'name': name})
         partner_id = partner_ids[0]
     return partner_id