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
=== modified file 'account_banking_nl_abnamro/'
--- account_banking_nl_abnamro/ 2012-10-27 19:53:50 +0000
+++ account_banking_nl_abnamro/ 2013-04-17 14:26:28 +0000
@@ -155,28 +155,55 @@
155 The string consists of slash separated KEY/VALUE pairs,155 The string consists of slash separated KEY/VALUE pairs,
156 but the slash is allowed to and known to occur in VALUE as well!156 but the slash is allowed to and known to occur in VALUE as well!
157 """157 """
158 items = field[1:].split('/') # skip leading slash158 def _sepa_message(field, reason):
159 return _(
160 'unable to parse SEPA string: %s - %s' % (field, reason))
162 def _get_next_key(items, start):
163 '''Find next key, starting from start, returns the key found,
164 the start position in the array and the end position + 1'''
165 known_keys = [
166 'TRTP', 'IBAN', 'BIC', 'NAME', 'RTRN', 'EREF', 'SWOC',
167 'REMI', 'ADDR', 'CPRP', 'CREF', 'CSID', 'ISDT', 'MARF',
168 'NRTX', 'NRTXR', 'PREF', 'PURP', 'REFOB', 'RREF', 'RTYP',
169 'SVCL', 'SWOD', 'BENM//ID', 'ORDP//ID', 'ORDP//RID',
171 'ULTB//NAME', 'ULTB//ID'
172 ]
173 items_len = len(items)
174 start_index = start
175 # Search until start after end of items
176 while start_index < items_len:
177 end_index = start_index + 1
178 while end_index < items_len:
179 key = '/'.join(items[start_index:end_index])
180 if key in known_keys:
181 return (key, start_index, end_index)
182 end_index += 1
183 start_index += 1
184 return False
186 items = field[1:].split('/')
187 assert len(items) > 1, _sepa_message(field, _('too few items'))
159 sepa_dict = {}188 sepa_dict = {}
160 prev_key = False189 item_index = 0
161 known_keys = ['TRTP', 'IBAN', 'BIC', 'NAME', 'RTRN', 'EREF',190 items_len = len(items)
162 'SWOC', 'REMI', ]191 key_info = _get_next_key(items, item_index)
163 while items:192 assert key_info, _sepa_message(
164 if len(items) == 1:193 field, _('no key found for start %d') % item_index)
165 raise osv.except_osv(194 assert key_info[1] == 0, _sepa_message(
166 _('Error !'),195 field, _('invalid data found before key %s') % key_info[0])
167 _("unable to parse SEPA string: %s") % field)196 while key_info:
168 key = items.pop(0)197 sepa_key = key_info[0]
169 if key not in known_keys:198 item_index = key_info[2]
170 # either an unknown key or a value containing a slash199 # Find where next key - if any - starts
171 if prev_key:200 key_info = _get_next_key(items, item_index)
172 sepa_dict[prev_key] = sepa_dict[prev_key] + '/' + key201 value_end_index = (key_info and key_info[1]) or items_len
173 else:202 sepa_value = (
174 raise osv.except_osv(203 ((value_end_index > item_index)
175 _('Error !'),204 and '/'.join(items[item_index:value_end_index]))
176 _("unable to parse SEPA string: %s") % field)205 or '')
177 else:206 sepa_dict[sepa_key] = sepa_value
178 sepa_dict[key] = items.pop(0).strip()
179 prev_key = key
180 return sepa_dict207 return sepa_dict
182 def parse_type(field):209 def parse_type(field):


People subscribed via source and target branches