← Back to team overview

banking-addons-team team mailing list archive

Re: [Merge] lp:~endiansolutions/banking-addons/ab61-nl_rabo into lp:banking-addons

 

Review: Needs Fixing

Hi Erwin,

thank you for this contribution. Here are my remarks:

l.9 Shared copyright between you and Credativ seem appropriate here.
l.40 As I have only recently learned myself, you can take GPL code and apply AGPL license to it. We will do so for 7.0. You might as well do it here now (note the licence mentioned in l.13).
l.38, 49 Please provide a name and description in English and a translation in the po file.

l.103 Please add a copyright notice.
l.105 This import is unused except in l.244 in raise_error which is unused in itself. Please remove the import as well as the raise_error method (I am aware the same error exists in HSBC).

l.162 Another copy/paste issue. Looks like you do have 'remote_account' (l.129), although maybe not for each transfer type.
l.160 Invalid transactions are only logged to the server log, invisible to the user who is importing a bank statement. Please set self.error_message in is_valid itself.

l.249,250 Please mention Rabobank here in the name and code, as you do not seem to aim at a generic 'MT940' parser here.

l.268 Dubious indentation. Align with the first 'r' in records in the previous line instead
l.266 Dubious indentation. Align with the 'p' in parser in the previous line instead.
l.278 Dummy list creation. How about something like:

    for r in filter(None, records):
        stmnt.import_record(r)

l.294 Looks like it's rather based on the HSBC parser than Fi Patu parser. In fact, the changes are so minimal (and half of these are just style changes) that you could consider a shared base and inherit that. At least add a copyright notice. I suggest shared copyrights between Credativ and yourself (if not the author of fi_patu).

Cheers,
Stefan.

-- 
https://code.launchpad.net/~endiansolutions/banking-addons/ab61-nl_rabo/+merge/141149
Your team Banking Addons Team is subscribed to branch lp:banking-addons.


References