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

[Ronald Portier (Therp)]

Make it possible to import abnamro statement files with extended sepa attributes in line.

Taking into account the remarks by Stefan I streamlined the code , hopefully solving the errors.

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

Thanks for fixing this!

l.8,28 Maybe combine in a single dictionary of possible keys?
l.62 Looks like the last key/value pair is not stored in sepa_dict. You could copy this line after the end of the 'while' loop.

review: Needs Fixing

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

I'm afraid your solution fails when the separator occurs in the field value as is allowed by the specs. E.g.

'/TRTP/SEPA OVERBOEKING/IBAN/CH999999999999F/BIC/UBSWCHZH80A/NAME/ORIG/INAL ART/REMI/CH9999999999/EREF/NOTPROVIDED/O\
RDP//ID/UBS AG'

Comes out as

{'ORIG//REMI': 'CH9999999999', 'NAME': '', 'BIC': 'UBSWCHZH80A', 'TRTP': 'SEPA OVERBOEKING', 'EREF': 'NOTPROVIDED', 'ORDP//ID': 'UBS AG', 'IBAN': 'CH999999999999F'}

when using your test script.

review: Needs Fixing

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

What is the status of this merge? Are the issues pointed out by Stefan corrected?

review: Needs Information

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

Sorry for the delay. I would still like to get the chance to test this but I had not had the time yet.

[Ronald Portier (Therp) (rportier1962)]

Hello Guewen,

The issues pointed out by Stefan have been corrected by revision 163 in this branch. This revision did bring with it a wholesale restructuring of the fix proposed.

Kind regards,

Ronald

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

Ronald,

thank you for the refactoring. I am very happy with the current algoritm. Sorry to bother you one more time, but I just have a hard time figuring out what 've' stands for. Using descriptive variables is almost always a good idea. Can you replace this variable name with something that is immediately clear? Please relace 'si' and 'ei' too, even though I managed to figure those out.

review: Needs Fixing

[lp:~therp-nl/banking-addons/6.1_lp1117319_abnamro_sepa_line]

164. By Ronald Portier (Therp) on 2013-04-17

[FIX] More meaningfull names for index variable names.

Unmerged revisions

=== modified file 'account_banking_nl_abnamro/abnamro.py'
--- account_banking_nl_abnamro/abnamro.py	2012-10-27 19:53:50 +0000
+++ account_banking_nl_abnamro/abnamro.py	2013-04-17 14:26:28 +0000
@@ -155,28 +155,55 @@
             The string consists of slash separated KEY/VALUE pairs,
             but the slash is allowed to and known to occur in VALUE as well!
             """
-            items = field[1:].split('/') # skip leading slash
+            def _sepa_message(field, reason):
+                return _(
+                    'unable to parse SEPA string: %s - %s' % (field, reason))
+
+            def _get_next_key(items, start):
+                '''Find next key, starting from start, returns the key found,
+                the start position in the array and the end position + 1'''
+                known_keys = [
+                    'TRTP', 'IBAN', 'BIC', 'NAME', 'RTRN', 'EREF', 'SWOC',
+                    'REMI', 'ADDR', 'CPRP', 'CREF', 'CSID', 'ISDT', 'MARF',
+                    'NRTX', 'NRTXR', 'PREF', 'PURP', 'REFOB', 'RREF', 'RTYP',
+                    'SVCL', 'SWOD', 'BENM//ID', 'ORDP//ID', 'ORDP//RID',
+                    'ORIG//CSID', 'ORIG//MARF', 'ULTD//NAME', 'ULTD//ID',
+                    'ULTB//NAME', 'ULTB//ID'
+                ]
+                items_len = len(items)
+                start_index = start
+                # Search until start after end of items
+                while start_index < items_len:
+                    end_index = start_index + 1
+                    while end_index < items_len:
+                        key = '/'.join(items[start_index:end_index])
+                        if  key in known_keys:
+                            return (key, start_index, end_index)
+                        end_index += 1
+                    start_index += 1
+                return False
+
+            items = field[1:].split('/')
+            assert len(items) > 1, _sepa_message(field, _('too few items'))
             sepa_dict = {}
-            prev_key = False
-            known_keys = ['TRTP', 'IBAN', 'BIC', 'NAME', 'RTRN', 'EREF',
-                          'SWOC', 'REMI', ]
-            while items:
-                if len(items) == 1:
-                    raise osv.except_osv(
-                        _('Error !'),
-                        _("unable to parse SEPA string: %s") % field)
-                key = items.pop(0)
-                if key not in known_keys:
-                    # either an unknown key or a value containing a slash
-                    if prev_key:
-                        sepa_dict[prev_key] = sepa_dict[prev_key] + '/' + key
-                    else:
-                        raise osv.except_osv(
-                            _('Error !'),
-                            _("unable to parse SEPA string: %s") % field)
-                else:
-                    sepa_dict[key] = items.pop(0).strip()
-                    prev_key = key
+            item_index = 0
+            items_len = len(items)
+            key_info = _get_next_key(items, item_index)
+            assert key_info, _sepa_message(
+                field, _('no key found for start %d') % item_index)
+            assert key_info[1] == 0, _sepa_message(
+                field, _('invalid data found before key %s') % key_info[0])
+            while key_info:
+                sepa_key = key_info[0]
+                item_index = key_info[2]
+                # Find where next key - if any - starts
+                key_info = _get_next_key(items, item_index)
+                value_end_index = (key_info and key_info[1]) or items_len
+                sepa_value = (
+                    ((value_end_index > item_index)
+                    and '/'.join(items[item_index:value_end_index]))
+                    or '')
+                sepa_dict[sepa_key] = sepa_value
             return sepa_dict
 
         def parse_type(field):