Merge lp:~yann-papouin/ocb-addons/6.1-bug-1267845-product-and-category-search-improved-usability into lp:ocb-addons/6.1

[Yann Papouin]

[Holger Brunn (Therp) (hbrunn)]

18: add spaces between operators

21: this seems to be one line too much

33: that's not safe against searching for ' / ' or the like, I'd wrap that into an
if category_names:

33, 36: shouldn't strip(product_category_separator + ' ') be enough? (the space just to be safe if we ever change product_category_separator)

42: if ids and classic_ids are nondistinct sets and there's a limit, you'll get less than limit ids, which is particularly annoying for name_search. I think you should pass limit+len(ids), remove duplicates in a manner that preserves class_ids' order and then shorten it as necessary

53: why is this necessary? And if so, why only if name is set?

63: can't you integrate that in the regex above? If you'll only have '[bla' trigger searching for the default code, but not 'bla]', adding a '?' after the closing square bracket would be enough. (and I can't really think of a use case for 'bla]').

64: if you don't agree with 63, .strip('[] ') should be enough here

review: Needs Fixing

[lp:~yann-papouin/ocb-addons/6.1-bug-1267845-product-and-category-search-improved-usability]

6828. By Yann Papouin on 2014-01-24

Check category_names length before accessing by index
Simpler strip
classic_ids are not duplicates of ids anymore
Reorder only if classic_ids if not empty

6829. By Yann Papouin on 2014-01-24

Simpler strip for product name_search

[Yann Papouin (yann-papouin)]

Thank you for your advices.

There is no more duplicates since ids are filtered by postgresql.
I added a manual result reorder because OpenERP "read" function does not return results in the ids order. This reordering is only necessary when a concatenation exists (via classics_ids)

About the regex, I tried to add a '?' but it's not working, it always returns an empty value

[Holger Brunn (Therp) (hbrunn)]

Yes, letting postgres do that is simpler and faster I guess.

Concerning the regex:

>>> import re
>>> re.compile('(\[(.*?)\]?)').search('[bla')
<_sre.SRE_Match object at 0x7f0be1590558>

That seems to work for me.

Looks good otherwise!

[Yann Papouin (yann-papouin)]

I'm getting following results:

>>> re.compile('(\[(.*?)\]?)').search('[bla').group(1)
'['
>>> re.compile('(\[(.*?)\]?)').search('[bla').group(2)
''
>>> re.compile('(\[(.*?)\]?)').search('[bla] bob').group(1)
'['
>>> re.compile('(\[(.*?)\]?)').search('[bla] bob').group(2)
''
>>> re.compile('(\[(.*?)\])').search('[bla] bob').group(1)
'[bla]'
>>> re.compile('(\[(.*?)\])').search('[bla] bob').group(2)
'bla'

[Holger Brunn (Therp) (hbrunn)]

You're right. This one should cover them all:
re.compile('(\[([^]]+)\]?)')

[Yann Papouin (yann-papouin)]

Indeed, it covers previous cases, but it fails if a close bracket exists in 'default_code' or 'name'.

>>> re.compile('(\[([^]]+)\]?)').search('[Code[A][B][C]] Product name').group(2)
'Code[A'

[Holger Brunn (Therp) (hbrunn)]

which gives the same result as the original regex

>>> re.compile('(\[(.*?)\])').search('[Code[A][B][C]] Product name').group(2)
'Code[A'

and the code path you added would search for 'Code[A][B][C' which doesn't seem that much better to me.

Don't you think that's a quite theoretical case?

[Holger Brunn (Therp) (hbrunn)]

How about that one?

review: Needs Information

[lp:~yann-papouin/ocb-addons/6.1-bug-1267845-product-and-category-search-improved-usability]

6830. By Yann Papouin on 2014-03-03

More generic regex for product name_search
Extended search based on name added if limit is not reached
Reorder if extended_ids is not empty

[Yann Papouin (yann-papouin)]

New commit with your regex in place.

To mimic the already existing behaviour, I've added an extended search based on name.

Note that contrary to the original code, I'm not using a python set because it is not ordered, I'm just using postgresql to filter the second query by removing previously found IDs. A sorted operation is done at the end, after the name_get.

Since this part of the code is really close to the existing code, maybe a mix between these two parts could be considered.

Unmerged revisions

6830. By Yann Papouin on 2014-03-03

More generic regex for product name_search
Extended search based on name added if limit is not reached
Reorder if extended_ids is not empty

6829. By Yann Papouin on 2014-01-24

Simpler strip for product name_search

6828. By Yann Papouin on 2014-01-24

Check category_names length before accessing by index
Simpler strip
classic_ids are not duplicates of ids anymore
Reorder only if classic_ids if not empty

6827. By Yann Papouin on 2014-01-10

Trim extra characters when searching for product and product categories for better usability

=== modified file 'product/product.py'
--- product/product.py	2013-11-20 01:17:39 +0000
+++ product/product.py	2014-03-03 13:12:34 +0000
@@ -193,6 +193,8 @@
 #----------------------------------------------------------
 class product_category(osv.osv):
 
+    product_category_separator = ' / '
+    
     def name_get(self, cr, uid, ids, context=None):
         if not len(ids):
             return []
@@ -201,9 +203,45 @@
         for record in reads:
             name = record['name']
             if record['parent_id']:
-                name = record['parent_id'][1]+' / '+name
+                name = record['parent_id'][1] + self.product_category_separator + name
             res.append((record['id'], name))
         return res
+    
+    def name_search(self, cr, user, name='', args=None, operator='ilike', context=None, limit=100):
+        if not args:
+            args = []
+        classic_ids = []
+        if name:
+            clean_name = False
+            category_args = []
+            if self.product_category_separator in name:
+                # Get a list of each category names and remove extra spaces 
+                category_names = [x for x in name.split(self.product_category_separator) if x.strip()]
+                # Use only latest value and remove interferences
+                if category_names:
+                    clean_name = category_names[-1].strip(self.product_category_separator + ' ')
+                    # Extract parent name for better accuracy
+                    if len(category_names) > 1:
+                        parent_name = category_names[-2].strip(self.product_category_separator + ' ')
+                        category_args = [('parent_id.name', '=', parent_name)]  
+                            
+            ids = self.search(cr, user, [('name', operator, clean_name or name)] + category_args + args, 
+                              limit=limit, context=context)
+            # Get remaining (classic) results (using clean name if exists) and avoid duplicates 
+            if not limit or len(ids) < limit:
+                classic_ids = self.search(cr, user, [('name', operator, clean_name or name),  ('id', 'not in', ids)], 
+                                          limit=(limit-len(ids) if limit else limit), context=context)    
+                if classic_ids:
+                    ids = ids + classic_ids   
+        else:
+            ids = self.search(cr, user, args, limit=limit, context=context)
+            
+        # Get named values
+        result = self.name_get(cr, user, ids, context=context)
+        # Re-order data by search priority since name_get lost our order (ids first, classic_ids afterward)
+        if classic_ids:
+            result = sorted(result, lambda x,y: cmp(ids.index(x[0]), ids.index(y[0])))
+        return result
 
     def _name_get_fnc(self, cr, uid, ids, prop, unknow_none, context=None):
         res = self.name_get(cr, uid, ids, context=context)
@@ -600,6 +638,7 @@
     def name_search(self, cr, user, name='', args=None, operator='ilike', context=None, limit=100):
         if not args:
             args = []
+        extended_ids = []
         if name:
             ids = self.search(cr, user, [('default_code','=',name)]+ args, limit=limit, context=context)
             if not ids:
@@ -616,13 +655,21 @@
                     ids.update(self.search(cr, user, args + [('name',operator,name)], limit=(limit-len(ids) if limit else limit) , context=context))
                 ids = list(ids)
             if not ids:
-                ptrn = re.compile('(\[(.*?)\])')
+                ptrn = re.compile('(\[([^]]+)\]?)')
                 res = ptrn.search(name)
                 if res:
-                    ids = self.search(cr, user, [('default_code','=', res.group(2))] + args, limit=limit, context=context)
+                    ids = self.search(cr, user, [('default_code', '=', res.group(2))] + args, limit=limit, context=context)
+                if not limit or len(ids) < limit:
+                    res = [x for x in ptrn.split(name) if x]
+                    if res:
+                        extended_ids = self.search(cr, user, [('name', operator, res[-1].strip('[] ')), ('id', 'not in', ids)] + args, limit=(limit-len(ids) if limit else limit), context=context)
+                        ids += extended_ids
         else:
             ids = self.search(cr, user, args, limit=limit, context=context)
         result = self.name_get(cr, user, ids, context=context)
+        # Re-order data by search priority since name_get lost our order (ids first, extended_ids afterward)
+        if extended_ids:
+            result = sorted(result, lambda x,y: cmp(ids.index(x[0]), ids.index(y[0])))
         return result
 
     #