Merge lp:~therp-nl/banking-addons/7.0-abnamro-sepa_type_case into lp:banking-addons

[Stefan Rijnhart (Opener)]

This proposal, upon approval, should preferably be merged with 6.1 too by means of cherrypicking

    bzr branch lp:banking-addons/6.1 ba61
    cd ba61
    bzr merge lp:~therp-nl/banking-addons/7.0-abnamro-sepa_type_case -r 189..190

[Stéphane Bidoul (Acsone) (sbi)]

LGTM.

BTW, which one is better?

sepa_type = sepa_dict.get('TRTP') or ''
sepa_type = sepa_dict.get('TRTP', '')

-sbi

review: Approve (code review, no test)

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

Thanks for the review!

I would always go for the latter in cases like:

    bar = foo.get('bar')
    if bar:
        ...

But as I was going to run a string operation on the result, I didn't want to run the risk of sepa_dict containing a key 'TRTP' with value False (even if the preceding code currently seems to guarantee that when sepa_dict contains a key 'TRTP', the value will always be a string)

[Stéphane Bidoul (Acsone) (sbi)]

Yes indeed, they are not equivalent. Getting tired :)

-sbi

On Wed, Oct 2, 2013 at 7:13 PM, Stefan Rijnhart (Therp) <email address hidden>wrote:

> Thanks for the review!
>
> I would always go for the latter in cases like:
>
> bar = foo.get('bar')
> if bar:
> ...
>
> But as I was going to run a string operation on the result, I didn't want
> to run the risk of sepa_dict containing a key 'TRTP' with value False (even
> if the preceding code currently seems to guarantee that when sepa_dict
> contains a key 'TRTP', the value will always be a string)
> --
>
> https://code.launchpad.net/~therp-nl/banking-addons/7.0-abnamro-sepa_type_case/+merge/188782
> You are reviewing the proposed merge of
> lp:~therp-nl/banking-addons/7.0-abnamro-sepa_type_case into
> lp:banking-addons.
>

[Holger Brunn (Therp) (hbrunn)]

review: Approve (code review)

=== modified file 'account_banking_nl_abnamro/abnamro.py'
--- account_banking_nl_abnamro/abnamro.py	2013-06-04 14:56:36 +0000
+++ account_banking_nl_abnamro/abnamro.py	2013-10-02 09:00:52 +0000
@@ -2,7 +2,7 @@
 ##############################################################################
 #
 #    Copyright (C) 2009 EduSense BV (<http://www.edusense.nl>)
-#                  2011 Therp BV (<http://therp.nl>)
+#                  2011 - 2013 Therp BV (<http://therp.nl>)
 #
 #    All Rights Reserved
 #
@@ -256,9 +256,9 @@
 
         if self.transfer_type == 'SEPA':
             sepa_dict = get_sepa_dict(''.join(fields))
-            sepa_type = sepa_dict.get('TRTP')
-            if sepa_type != 'SEPA OVERBOEKING':
-                raise ValueError,_('Sepa transaction type %s not handled yet')
+            sepa_type = sepa_dict.get('TRTP') or ''
+            if sepa_type.upper() != 'SEPA OVERBOEKING':
+                raise ValueError, _('Sepa transaction type %s not handled yet') % sepa_type
             self.remote_account = sepa_dict.get('IBAN',False)
             self.remote_bank_bic = sepa_dict.get('BIC', False)
             self.remote_owner = sepa_dict.get('NAME', False)