Hi Stefan,

Thanks for your answers. I've adapted some changes to ABF in order to 
address some of your remarks.
Most notably:

1. Adapted German localized BBAN format
2. Changed model members to default to str (makes 
isinstance(model.member, (str, unicode)) == True, even when untouched)
3. Reactivated apparently untested path in wizard.
4. Added more remarks to parser template regarding numbering issues 
(prevent future discussions)

Can you please pull these, remove your visual/style changes and create a 
new merge proposal?

One question remains: What is your definition of "poor performance" 
regarding the online Dutch IBAN/BIC convertor?
BTW, I'm not sure if paying will enhance the data quality. Most paid 
services simply calculate their responses.

With your new proposal, I will check ABF's cooperation with 
vouchers/payments. I'm interested to see how it behaves.
I was already planning on updating ABF to face the new semantics. This 
looks like a welcome jump start.
BTW, I'll start a new branch for this, in order to allow backports to v5.

TIA,
--
Pieter J. Kersten
*T:* +31 630 230208
*KvK:* 08178777
Logo EduSense B.V. <http://www.edusense.nl>

Op 28-04-11 08:55 schreef Stefan Rijnhart (Therp):
> Hi Pieter,
>
> again, thank you for your fast and perceptive response! I agree with
> most of your remarks. Please find some additional comments below.
>
>
> On 27-04-11 12:30, Pieter J. Kersten (EduSense BV) wrote:
>> Generic:
>>       I can see a lot of technical improvements for v6. However, there were
>>       more changes than just the technical ones. For starters, v6 introduced
>>       the voucher layer between account and account_payment. I'm missing
>>       this in your proposal. Is this code used in production? If so,
>>       what are the experiences? How does it coexist/cooperate with the
>>       payment/voucher system?
> This code is used in a couple of small setups. We knowingly ignored
> account_voucher for this version in order to simplify verification of
> the operation of the migrated code. The process of payments (through
> payment orders) and bank reconciliation works fine without it, but a
> logical next step would be looking at integrating account_voucher.
>
>> account_banking/account_banking.py: @-1218,+1210 -1239,+1231:
>>       Not sure if this is an improvement. I'll leave it for now.
>>
> We felt the error was a bit too harsh on the account number, given the
> poor performance of the online iban service for Dutch bank accounts. It
> is a matter of taste. We will be looking at integrating a paid service
> for iban/bic lookups.
>
>> account_banking/sepa/iban.py:
>>       This change is not conform specs. Any sources I can check for this?
>>       Will leave it untill more info arrives.
> This change was triggered by a real transaction. We looked to Wikipedia
> for confirmation
> (http://en.wikipedia.org/wiki/International_Bank_Account_Number lists
> German bbank as 18n, no check digit). Ecbs has the length listed as 18!n
> as well (http://www.ecbs.org/iban/germany-bank-account-number.html)
>
>> account_banking/wizard/bank_import/@@ -544,+545:
>>       This is unneeded. You don't use it anywhere. Won't take it.
>>
> No, it is used in line 931, although I did not introduce it.
>
>> account_banking/wizard/bank_import/@@ -795,+798:
>>       Can't see any value in this change. Just make sure that
>>           isinstance(transaction.remote_account, (str, unicode)) is True
>>       which must be done in the import parsers.
> In the ABNAmro filter the remote owner, but not the remote_account is
> retrieved in case of a payment terminal transaction. I would rather
> check here than having to initialize remote_account as an empty string.
>
>> account_banking/wizard/banktools/get_or_create_partner:
>>       Ok, backporting the unique construct to v5.
>>       Having some difficulties with the defensive field resolving code.
>>       Is this really needed? Never faced any problems with it.
> Yes, if we take requiredness into account, some checks are superfluous.
> I would suggest to just keep
>
>       user.company_id.partner_id.country and
> user.company_id.partner_id.country.id or False
>
>> account_banking/wizard/banktools/create_bank_account:
>>       What is the purpose of these changes?
>>       I can't see a semantic value for a catch-all 'UNKNOWN' bank.
>>       Problem is, when the bank gets known, chances are it won't be
>>       the same as "the other" accounts with UNKNOWN tied to it.
>>       Better leave it.
> Indeed, might be a mistake on my part regarding requiredness
>
> Regards,
> Stefan.
>
>