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

Proposed by Ronald Portier (Therp)
Status: Superseded
Proposed branch: lp:~therp-nl/banking-addons/6.1_lp1117319_abnamro_sepa_line
Merge into: lp:banking-addons/6.1
Diff against target: 80 lines (+48/-21)
1 file modified
account_banking_nl_abnamro/ (+48/-21)
To merge this branch: bzr merge lp:~therp-nl/banking-addons/6.1_lp1117319_abnamro_sepa_line
Reviewer Review Type Date Requested Status
Stefan Rijnhart (Opener) Needs Fixing
Guewen Baconnier @ Camptocamp Needs Information
Review via email:

This proposal supersedes a proposal from 2013-02-06.

This proposal has been superseded by a proposal from 2013-04-17.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote : Posted in a previous version of this proposal

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


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
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

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

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

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

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

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,


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


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
164. By Ronald Portier (Therp)

[FIX] More meaningfull names for index variable names.

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'account_banking_nl_abnamro/'
2--- account_banking_nl_abnamro/ 2012-10-27 19:53:50 +0000
3+++ account_banking_nl_abnamro/ 2013-04-17 14:26:28 +0000
4@@ -155,28 +155,55 @@
5 The string consists of slash separated KEY/VALUE pairs,
6 but the slash is allowed to and known to occur in VALUE as well!
7 """
8- items = field[1:].split('/') # skip leading slash
9+ def _sepa_message(field, reason):
10+ return _(
11+ 'unable to parse SEPA string: %s - %s' % (field, reason))
13+ def _get_next_key(items, start):
14+ '''Find next key, starting from start, returns the key found,
15+ the start position in the array and the end position + 1'''
16+ known_keys = [
17+ 'TRTP', 'IBAN', 'BIC', 'NAME', 'RTRN', 'EREF', 'SWOC',
18+ 'REMI', 'ADDR', 'CPRP', 'CREF', 'CSID', 'ISDT', 'MARF',
19+ 'NRTX', 'NRTXR', 'PREF', 'PURP', 'REFOB', 'RREF', 'RTYP',
20+ 'SVCL', 'SWOD', 'BENM//ID', 'ORDP//ID', 'ORDP//RID',
22+ 'ULTB//NAME', 'ULTB//ID'
23+ ]
24+ items_len = len(items)
25+ start_index = start
26+ # Search until start after end of items
27+ while start_index < items_len:
28+ end_index = start_index + 1
29+ while end_index < items_len:
30+ key = '/'.join(items[start_index:end_index])
31+ if key in known_keys:
32+ return (key, start_index, end_index)
33+ end_index += 1
34+ start_index += 1
35+ return False
37+ items = field[1:].split('/')
38+ assert len(items) > 1, _sepa_message(field, _('too few items'))
39 sepa_dict = {}
40- prev_key = False
41- known_keys = ['TRTP', 'IBAN', 'BIC', 'NAME', 'RTRN', 'EREF',
42- 'SWOC', 'REMI', ]
43- while items:
44- if len(items) == 1:
45- raise osv.except_osv(
46- _('Error !'),
47- _("unable to parse SEPA string: %s") % field)
48- key = items.pop(0)
49- if key not in known_keys:
50- # either an unknown key or a value containing a slash
51- if prev_key:
52- sepa_dict[prev_key] = sepa_dict[prev_key] + '/' + key
53- else:
54- raise osv.except_osv(
55- _('Error !'),
56- _("unable to parse SEPA string: %s") % field)
57- else:
58- sepa_dict[key] = items.pop(0).strip()
59- prev_key = key
60+ item_index = 0
61+ items_len = len(items)
62+ key_info = _get_next_key(items, item_index)
63+ assert key_info, _sepa_message(
64+ field, _('no key found for start %d') % item_index)
65+ assert key_info[1] == 0, _sepa_message(
66+ field, _('invalid data found before key %s') % key_info[0])
67+ while key_info:
68+ sepa_key = key_info[0]
69+ item_index = key_info[2]
70+ # Find where next key - if any - starts
71+ key_info = _get_next_key(items, item_index)
72+ value_end_index = (key_info and key_info[1]) or items_len
73+ sepa_value = (
74+ ((value_end_index > item_index)
75+ and '/'.join(items[item_index:value_end_index]))
76+ or '')
77+ sepa_dict[sepa_key] = sepa_value
78 return sepa_dict
80 def parse_type(field):


People subscribed via source and target branches