Merge lp://staging/~acsone-openerp/banking-addons/ba-70-payment-export-refactoring into lp://staging/banking-addons

Would it be better to move the following to account_direct_debit?
- payment_term_ids field in payment.mode
- payment_order_type field on payment.mode.type
- payment_order_create.py

This would focus this module account_banking_payment_export on its real purpose, which is adding the hook for payment export wizards.

BTW, how does this all relate to lp:account-payment/account_payment_extension? #headache

Hi Stéphane,

Thanks for this contrib, this is a good first step in merging this project into one ! I reviewed the structure/concept and it seems good to me. No deep look on the Python code though.

Regards,

Joël

Hi,

Since the purpose of account_banking_payment_export module was still difficult to explain, I went back a little bit and reduced its scope to providing the payment export infrastructure, including the sample "manual" payment mode type, and nothing more.

It's now easy to grasp it's purpose. Other features from account_banking(_payment) can be split in other modules later.

While I was at it I made the manual payment wizard a bit nicer for 7.0.

-sbi

Documented the conflict with lp:account-payment/account_payment_exention.

Thanks for your hard work on that! I'm not enough familiar to speak about the merge in itself, but from what I can understand, sounds good.

> Thanks for your hard work on that! I'm not enough familiar to speak about the
> merge in itself, but from what I can understand, sounds good.

s/merge/split/

Hi Stéphane,

thank you for your great work and apologies for the late review. My first impression is that the refactoring is very solid. Also, and importantly, upgrading any existing installation of banking-addons should be straightforward because of the dependency structure, as long as the new module is in the addons path.

Of course, there are a few details to discuss:

- Having account_banking_payment installed, rerouting signal done to state 'send' works when exporting the payment order, but when the reconciliation in account_banking attempts to finish the payment order workflow using the same signal, the payment order stays in the 'send' state. I'm sure there is a way to get this to work using the workflow engine, but depending on whether you are a fan of the workflow engine you could also consider creating a method on the payment order model that triggers 'sent' in account_banking_payment and 'done' in account_banking_payment_export.

- As far as I know, there is no way to craft a migration script that guarantees to move an xml-id to a *newly* installed module that has no common dependency with the originating module. You should probably keep the old module name as part of the manual order type xml-id, and we'll move it back on the next OpenERP major release migration.
- 'name' in the __openerp__.py manifest is the same for both payment modules, which makes it hard to distinguish in the interface

- There is a missing dependency on base_iban, due to the bank type specified in the manual payment mode type. As depending on base_iban is a little Eurocentric, you could consider making the payment mode types editable in the interface (and its data 'noupdate') + let the SEPA export module depend on base_iban itself (or depend on base_iban now and we'll keep the latter in mind).

- Are you very strongly against keeping our version line2bank in the export module? In the days of transitioning to SEPA, where multiple account numbers per partner are not uncommon, I think this is going to hurt a number of people. One option is to at least refactor it into a separate module. But I would prefer to have it in the export module for simplicity.

Not for this review but on a related note, I want to implement a general check on the presence of an account number in each line. Would you agree to have me put this in the export module once it is merged? It is after all, essential for the export of any order to a bank. Alexis already resorted to putting one in the SEPA module itself, but it is useful for all export formats.

Thanks for the review, Stefan.

- I'll look into the workflow issue. It's not immediately obvious to me why
it does not work.

- regarding the xml_id, if I keep "account_banking.manual_bank_transfer",
it requires to have account_banking installed, which is a dependency we
want to avoid. Or do I overlook something?

- I'm fine with the dependency on base_iban

- with line2bank, do you mean the payment_order_create class? I left it
alone because it was pulling the payment_term filter which is unrelated to
the pure payment export functionality (and also because I was not too sure
of its purpose). Does it make sense to keep the
payment_order_create.create_payment() method in account_payment_export and
leave the rest in account_payment? Perhaps you can help me explaining it's
purpose in the module manifest.

Regarding the check for the presence of an account number, I'm fine with
that.

In the same line of thought, I also saw that Alexis put a feature to
preserve the history of exported files in the sepa module. That's also a
generic requirement that could live in account_payment_export.

-sbi

On Sat, Sep 7, 2013 at 3:40 PM, Stefan Rijnhart (Therp) <email address hidden>wrote:

> Review: Needs Fixing
>
> Hi Stéphane,
>
> thank you for your great work and apologies for the late review. My first
> impression is that the refactoring is very solid. Also, and importantly,
> upgrading any existing installation of banking-addons should be
> straightforward because of the dependency structure, as long as the new
> module is in the addons path.
>
> Of course, there are a few details to discuss:
>
> - Having account_banking_payment installed, rerouting signal done to state
> 'send' works when exporting the payment order, but when the reconciliation
> in account_banking attempts to finish the payment order workflow using the
> same signal, the payment order stays in the 'send' state. I'm sure there is
> a way to get this to work using the workflow engine, but depending on
> whether you are a fan of the workflow engine you could also consider
> creating a method on the payment order model that triggers 'sent' in
> account_banking_payment and 'done' in account_banking_payment_export.
>
> - As far as I know, there is no way to craft a migration script that
> guarantees to move an xml-id to a *newly* installed module that has no
> common dependency with the originating module. You should probably keep the
> old module name as part of the manual order type xml-id, and we'll move it
> back on the next OpenERP major release migration.
> - 'name' in the __openerp__.py manifest is the same for both payment
> modules, which makes it hard to distinguish in the interface
>
> - There is a missing dependency on base_iban, due to the bank type
> specified in the manual payment mode type. As depending on base_iban is a
> little Eurocentric, you could consider making the payment mode types
> editable in the interface (and its data 'noupdate') + let the SEPA export
> module depend on base_iban itself (or depend on base_iban now and we'll
> keep the latter in mind).
>
> - Are you very strongly against keeping our version line2bank in the
> export module? In the days of transitio...

On 09/07/2013 05:57 PM, Stéphane Bidoul (Acsone) wrote:
> Thanks for the review, Stefan.
>
> - I'll look into the workflow issue. It's not immediately obvious to me why
> it does not work.

Hi Stéphane,

Sorry, problem between keyboard and chair here, as I was doing the
reconciliation of the payment order (admittedly fueled by my
unfamiliarity of using the same workflow signal for multiple
transitions). It works!

> - regarding the xml_id, if I keep "account_banking.manual_bank_transfer",
> it requires to have account_banking installed, which is a dependency we
> want to avoid. Or do I overlook something?

Yes, apparently. I thought we could pull this one off, as official
modules do use references to other modules, combined with the fact that
internally you can assign arbitrary 'module' labels in ir_model_data.
But when I test it, the installation process complains that
"account_banking.manual_bank_tranfer" refers to an uninstalled module.

I'm not sure what to do. If I upgrade an existing setup that contains a
payment mode with this type, the process breaks with 'null value in
column "type" violates not-null constraint'.

> - with line2bank, do you mean the payment_order_create class? I left it
> alone because it was pulling the payment_term filter which is unrelated to
> the pure payment export functionality (and also because I was not too sure
> of its purpose).

It is actually not line2bank itself, but making it work properly. The
fix is twofold. First, it is reading suitable bank types from the
payment mode type. You kept this bit. The other part is passing the
payment order mode to line2bank. As you can see here, OpenERP does not
do this:
http://bazaar.launchpad.net/~openerp/openobject-addons/7.0/view/head:/account_payment/wizard/account_payment_order.py#L72.

> Does it make sense to keep the
> payment_order_create.create_payment() method in account_payment_export and
> leave the rest in account_payment? Perhaps you can help me explaining it's
> purpose in the module manifest.

Account_banking_payment overwrites payment_order_create.create_payment()
but you don't have to do that if you agree to apply the following trick:
wrap around create_payment to extract the payment mode id and put it in
the context + call super(). Then also wrap around line2bank and extract
the id from the context again, if the payment mode id passed to
line2bank is None + call super() with it.

(the payment term filter is an optional way to use the payment term to
preselect the manner of payment through partner settings or at invoicing
time. This is particularly useful for debit orders because of the
required mandates per partner)

Cheers,
Stefan.

One option to move the xml id could be to update the record in ir_model_data using an SQL query in account_banking_payment_export, in payment.mode.type's _auto_init() method. I think that would give it the right timing. It will run every time the module is upgraded which is ugly but the overhead is negligible.

I was considering that approach too. I'll give it a try.

-sbi
Le 8 sept. 2013 11:08, "Stefan Rijnhart (Therp)" <email address hidden> a écrit :

> One option to move the xml id could be to update the record in
> ir_model_data using an SQL query in account_banking_payment_export, in
> payment.mode.type's _auto_init() method. I think that would give it the
> right timing. It will run every time the module is upgraded which is ugly
> but the overhead is negligible.
>
> --
>
> https://code.launchpad.net/~acsone-openerp/banking-addons/ba-70-payment-export-refactoring/+merge/179543
> You proposed
> lp:~acsone-openerp/banking-addons/ba-70-payment-export-refactoring for
> merging.
>

Hi again,

Here is a new version.

I adressed all of Stefan's comments:
- module name
- base_iban dependency
- migration of manual_bank_transfer xml id
- the trick/hack/thing about making line2bank receive the payment mode :)

@Stefan, have you ever attempted to integrate that last one in a MP to the core?
The fix there would be simple, although difficult to explain in a bug report.

plus:
- a bit of pep8
- a view bug I discovered while testing

Thanks. The code looks good. One nit:

l.1286: would it be alright with you to put the changes to the move line model in a separate file, following the (relatively recent) conventions in this series of modules?

I would still like a couple of days for testing before I can definately approve, but it's looking really good.

About line2bank in the core: it's just a remnant after payment.mode.type was removed from the core in OpenERP 6.0 so the proper fix would be to replace it in the core with a method that transparently selects the partner's first bank account. I don't see what this project wins with that, as a hack like the one we came up with would still be needed.

On Fri, Sep 13, 2013 at 9:57 AM, Stefan Rijnhart (Therp) <email address hidden>wrote:

> l.1286: would it be alright with you to put the changes to the move line
> model in a separate file, following the (relatively recent) conventions in
> this series of modules?
>

I considered that too, but in the end I decided that since the two parts of
the hack where so closely related that it would make the code much harder
to understand if it was split in two files. After all, python never
enforced a one-class-per-file rule like java, and in this case it made
sense to keep them together. What do you think?

-sbi

Good point, let's keep them together.

Stéphane,

once again thank you for your patience. I have succesfully tested migrating existing databases to the current state of the branch. Meanwhile, I am writing a lengthy test that simulates a payment order - bank statement round trip, but that should not keep back the merge of this branch.

In the last revisions, a slight error showed up: BaseModel._auto_init() actually returns something that can be meaningful. It does not seem to make a difference here but I think it would be best to return the result of super() after line 1226.

And then we're done.

Hi Stefan,

Done. Thanks for the extensive review.

-sbi

Approved. Thank you for pushing the idea of decoupling the functionalities and working so hard on it! If no one else has any more comments this could be merged early next week.