Merge lp://staging/~luc-demeyer/account-financial-tools/7.0-account_pain into lp://staging/~account-core-editors/account-financial-tools/7.0

Hi, Luc, maybe account-payment repository is better for this module?

Regards.

@Pedro

I think SEPA-related stuff belongs to the banking-addons https://launchpad.net/banking-addons

By the way, there is already a module for SEPA Credit Transfer in the banking-addons (account_banking_sepa_credit_transfer), so it would be interesting to see if there are some features that are in this module but are not in the module account_banking_sepa_credit_transfer and merge the features of the 2 modules.

Hello Luc,

cool to see Noviat joining the OCA effort. I won't comment functionally but certainly the remarks of Alexis and Pedro are valid.

Technically, there a few caveats that the watchdogs will remind you if I don't, so let's do it:

* please use orm.Model instead of osv.osv which is deprecated now
* also use orm.TransientModel instead of osv.memory
* also you use context={} in methods definitions at lines 309, 324, 327 and 458. You better use the ugly context=None + test against None plumbing instead. Indeed the mutable {} would eventually remain from a method call to another, see http://effbot.org/zone/default-values.htm eventually (Hey but I guess you knew it already).

Regards.

Hi Luc,

I gladly welcome the initiative of Noviat of bringing their great modules under the umbrella of the community projects!

With regards to this particular module, I must agree with Alexis that his module overlaps greatly with this module. Now before Stéphane Bidoul made a great effort to decouple the banking addons export modules from the whole bank statement import and reconciliation bit, there would have been a legitimate place for a standalone module like this. But since Alexis' SEPA export module can now also be used with only a very thin boilerplate architecture (account_banking_payment_export), I have to disagree with adopting this module too.

Looking at what this module adds is as announced, heavily biased for Belgium. This is awkward in a module that has potential use in all of Europe, but it would be perfectly alright to refactor them out into a module on top of account_banking_sepa_credit_transfer. Additionally, I see that you allow sale refunds to be selected in the payment order. Given its general usefulness, this should probably be adopted in account_banking_payment_export. Same for improving support for multi-currency environments.

I hope that my review is not discouraging you. Your input is much appreciated, but the point of the community repositories is also to combine focus and avoid double work.

Cheers,
Stefan.

Hi, I told about account-payment repository because I see that this feature is developed following account-payment approach instead of banking-payment, the same as, for example, spanish bank formats export.

Maybe the two approaches can coexist?

Regards.

Hello Pedro,

just to mention: in Brazil we are also account-payment biased so far (we
were inspired by NaN module like 4 years ago now). And we still have to
develop SEPA like format but for Brazil (CNAB). So we have more or less the
same problem as you. I'm not sure yet what approach is the best, but we
should definitely keep convergence in mind with account-payment and
banking-payment. I also mention that it looks that NaN and Ziakzak recently
ported the account-payment approach to Tryton (so they stick we it).

Regards.

On Wed, Oct 23, 2013 at 6:25 AM, Pedro Manuel Baeza
<email address hidden>wrote:

> Hi, I told about account-payment repository because I see that this
> feature is developed following account-payment approach instead of
> banking-payment, the same as, for example, spanish bank formats export.
>
> Maybe the two approaches can coexist?
>
> Regards.
> --
>
> https://code.launchpad.net/~luc-demeyer/account-financial-tools/7.0-account_pain/+merge/192256
> You are reviewing the proposed merge of
> lp:~luc-demeyer/account-financial-tools/7.0-account_pain into
> lp:account-financial-tools.
>

I updated the https://code.launchpad.net/~luc-demeyer/account-financial-tools/7.0-account_pain branch with the technical changes documented below.

Luc

-----Original Message-----
From: <email address hidden> [mailto:<email address hidden>] On Behalf Of Raphaël Valyi - http://www.akretion.com
Sent: woensdag 23 oktober 2013 6:05
To: <email address hidden>
Subject: Re: [Merge] lp:~luc-demeyer/account-financial-tools/7.0-account_pain into lp:account-financial-tools

Review: Needs Fixing

Hello Luc,

cool to see Noviat joining the OCA effort. I won't comment functionally but certainly the remarks of Alexis and Pedro are valid.

Technically, there a few caveats that the watchdogs will remind you if I don't, so let's do it:

* please use orm.Model instead of osv.osv which is deprecated now
* also use orm.TransientModel instead of osv.memory
* also you use context={} in methods definitions at lines 309, 324, 327 and 458. You better use the ugly context=None + test against None plumbing instead. Indeed the mutable {} would eventually remain from a method call to another, see http://effbot.org/zone/default-values.htm eventually (Hey but I guess you knew it already).

Regards.

--
https://code.launchpad.net/~luc-demeyer/account-financial-tools/7.0-account_pain/+merge/192256
You are the owner of lp:~luc-demeyer/account-financial-tools/7.0-account_pain.

I am in favor of one SEPA community module in stead of multiple.

Hence the best approach would be to merge the functionality.
After a quick comparison module from Alexis I think it's no too hard to do.
We should merge the functionality which will require some changes to both the 'account_banking_sepa_credit_transfer' as the underlying 'account_banking_payment_export' and allow for SEPA localisation modules on top of the 'account_banking_sepa_credit_transfer'.
Once this is done, we can migrate our 'account_pain' install base to the new module. We are also working on new functionalities and these would than become more widely available.

Alexis, Stefan,

What about a physical meeting where we run through the requirements and after this produce a new version that can be used as well as the basis for other country specific SEPA extensions.
We can host such a session in our Brussels offices (centrally located between France and Holland, I'll pay dinner, lunch, drinks, ... to compensate for your travel costs).

Regards,
Luc

-----Original Message-----
From: <email address hidden> [mailto:<email address hidden>] On Behalf Of Raphaël Valyi - http://www.akretion.com
Sent: woensdag 23 oktober 2013 15:24
To: Pedro Manuel Baeza
Subject: Re: [Merge] lp:~luc-demeyer/account-financial-tools/7.0-account_pain into lp:account-financial-tools

Hello Pedro,

just to mention: in Brazil we are also account-payment biased so far (we were inspired by NaN module like 4 years ago now). And we still have to develop SEPA like format but for Brazil (CNAB). So we have more or less the same problem as you. I'm not sure yet what approach is the best, but we should definitely keep convergence in mind with account-payment and banking-payment. I also mention that it looks that NaN and Ziakzak recently ported the account-payment approach to Tryton (so they stick we it).

Regards.

On Wed, Oct 23, 2013 at 6:25 AM, Pedro Manuel Baeza
<email address hidden>wrote:

> Hi, I told about account-payment repository because I see that this
> feature is developed following account-payment approach instead of
> banking-payment, the same as, for example, spanish bank formats export.
>
> Maybe the two approaches can coexist?
>
> Regards.
> --
>
> https://code.launchpad.net/~luc-demeyer/account-financial-tools/7.0-ac
> count_pain/+merge/192256 You are reviewing the proposed merge of
> lp:~luc-demeyer/account-financial-tools/7.0-account_pain into
> lp:account-financial-tools.
>

--
https://code.launchpad.net/~luc-demeyer/account-financial-tools/7.0-account_pain/+merge/192256
You are the owner of lp:~luc-demeyer/account-financial-tools/7.0-account_pain.

Here is result of the Akretion-Noviat code sprint on SEPA credit transfer to add the additionnal features of the module account_pain inside the module account_banking_sepa_credit_transfer :

https://code.launchpad.net/~akretion-team/banking-addons/70-akretion-noviat-sepa-sprint

But before I propose this branch for a merge on lp:banking-addons, I will need to have the following MP approved : https://code.launchpad.net/~akretion-team/banking-addons/70-sepa-credit-transfer-update/+merge/194948