[16.0][BIG] Large refactoring/improvement/cleanup of OCA/bank-payment #1174
Conversation
NEW FEATURES
- activate 'in_payment' payment_state on invoices... at last !
- payment order numbering: add ability to have a sequence specific to a particular payment mode (optional)
- add show account number policy "first_last" to show N first and N last characters of bank account number
SMALL IMPROVEMENTS
- account.payment.method: rename field "payment_order_only" to "payment_order_ok" for naming consistency with the field of account.payment.mode. Auto-set "payment_order_ok" of payment mode from the equivalent field of payment method. Migration scripts provided for account.payment.method and the data of SEPA credit transfer and SEPA direct debit.
- payment order attachment: add M2O payment_file_id that points to the payment file. No more need for a simplified attachment form view.
- keep the spaces in the scrambled bank account number, so that the user easily understands that it is a "scrambled" bank account number
- add field acc_number_scrambled on res.partner.bank for easy and direct use of scrambled account number
SIMPLIFICATION / CODE CLEANUP
- remove field partner_bank_id on account.move.line
- move code for scrambled account number from qweb report to res.partner.bank class
- remove boolean field show_bank_account_from_journal from account.payment.mode: we now consider that this option is always true i.e. when account_payment_partner is installed, Odoo takes the bank accounts to display on the customer invoice from the payment mode (if the payment mode is empty, it displays all the bank accounts of the company). As a consequence, hide partner_bank_id on customer invoice form view.
- remove fields inbound_payment_order_only and outbound_payment_order_only from account.journal : these 2 fields were not used!
- add check_company=True where it was missing and remove code of constraint that checks company consistency
- re-organise form view of payment order to equilibrate between left column and right column
TECHNICAL:
- replace @api.onchange by computed readonly=False store=True fields
- remove (or reduce) the use of demo data in tests
- speed-up tests by using tracking_disable=True
- Replace (6, 0, []) and (0, 0, {}) by Command.set() and Command.create()
- add sql unicity constraint on payment order number per company
alexis-via
force-pushed
the
16-payment_order-improve_and_cleanup
branch
from
84402e5 to
6e7842a
Compare
alexis-via
force-pushed
the
16-payment_order-improve_and_cleanup
branch
from
6e7842a to
4584b38
Compare
|
It's the module account_payment_order_grouped_output which make the tests for payment_status in_payment vs paid fail. I have never used account_payment_order_grouped_output ; according to README, it has been developed to mitigate the drawbacks of the switch to account.payment !!! |
|
For some strange reasons that I don't understand, when you upgrade to this branch, Odoo fails because a view has been deleted in the source code. You can solve the issue by executing this SQL request: |
alexis-via
force-pushed
the
16-payment_order-improve_and_cleanup
branch
from
4584b38 to
81a7c2f
Compare
|
Tests are not green. |
|
Payment order, in_payment and bank statement reconciliation In this PR, I activate the "in_payment" payment status on invoice. For me, it's one of the major advantages of using the object account.payment, that was implemeted by @pedrobaeza in this big PR #979 One of the drawbacks in the switch to account.payment is that, when you validate a payment order with 100 different partners, you get 100 different entries in the Outstanding receipts account. So, when you have the bank statement line with the total amount of the payment order, you have to select 100 counter-part entries in the Outstanding receipts account !!! The module account_payment_order_grouped_output is designed to solve this problem:
I was told that, in the Enterprise module, they have a special tab in the bank statement reconcile interface for payments by lot: so, when you process the bank statement lines with the total amount of the payment order, Odoo proposes the payment orders not yet reconciled with the bank statement lines ; when you select the appropriate payment order, it will automatically set the 100 counterpart lines to reconcile the bank statement line. So, how should be move forward on this issue ? @pedrobaeza @etobella My opinion:
|
|
Yes, |
Harmonize values for reference_type on account.move and communication_type on account.payment.line: 2 possible values : free and structured (migration script provided) Simplify code for reference.
alexis-via
force-pushed
the
16-payment_order-improve_and_cleanup
branch
from
81a7c2f to
c7391e0
Compare
|
@alexis-via Can you rebase? |
| @@ -657,7 +654,7 @@ def generate_remittance_info_block(self, parent_node, line, gen_args): | |||
| creditor_ref_info_type_issuer = etree.SubElement( | |||
| creditor_ref_info_type, "Issr" | |||
| ) | |||
| creditor_ref_info_type_issuer.text = communication_type | |||
| creditor_ref_info_type_issuer.text = "ISO" | |||
There was a problem hiding this comment.
@alexis-via please keep communication_type. We need it for belgium where (IIRC) it must become bba.
https://github.com/OCA/l10n-belgium/tree/10.0/l10n_be_iso20022_pain
cc/ @luc-demeyer
There was a problem hiding this comment.
@sbidoul
I fully agree.
Small remark on l10n_be_iso20022_pain: should be 'BBA' (uppercase), cf. my comment on 16.0 PR.
There was a problem hiding this comment.
Thanks for pointing this out. Let's talk about this during the next code sprint, because the datamodel for structured communication type is really messy and unclean. I'm confident that we'll agree on a nice and clear datamodel on this, that will make the l10n_be_iso20022_pain simpler and make the XML generation code simpler.
|
@alexis-via |
|
Hi @luc-demeyer , which PR are you waiting for? Can we help to push this? Is blocking migration to v17 |
|
Please rebase and solve conflicts. |
|
I'll continue to work on this next month (but not in the coming days) |
| res.append((rec.id, "[%s] %s" % (rec.code, rec.name))) | ||
| return res | ||
|
|
||
| def _name_search( |
There was a problem hiding this comment.
Instead of this, you can just put _rec_names_search = ["code", "name"].
There was a problem hiding this comment.
Done. Thanks for your remark!
| @@ -0,0 +1,3 @@ | |||
| id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink | |||
| access_account_pain_regulatory_reporting_read,Read access on account.pain.regulatory.reporting,model_account_pain_regulatory_reporting,base.group_user,1,0,0,0 | |||
| access_account_pain_regulatory_reporting_full,Full access on account.pain.regulatory.reporting,model_account_pain_regulatory_reporting,account.group_account_manager,1,1,1,1 | |||
There was a problem hiding this comment.
Shouldn't be the full right access the "Payment" group?
| <field name="country_id" /> | ||
| <field name="active" invisible="1" /> | ||
| </group> | ||
| </form> |
There was a problem hiding this comment.
I let prettier do the formatting work and pre-commit doesn't give any error. Strange !
| @openupgrade.migrate() | ||
| def migrate(env, version): | ||
| sct_method = env.ref("account_banking_sepa_credit_transfer.sepa_credit_transfer") | ||
| sct_method.write({"payment_order_ok": True}) |
There was a problem hiding this comment.
Don't force this. Just rename the column of the field on the source one.
There was a problem hiding this comment.
But the payment method for SEPA credit transfer should always have payment_order_ok = true, no ? Otherwise, what's the point of installing account_banking_sepa_credit_transfer ??
| if line_purpose: | ||
| purpose = etree.SubElement(credit_transfer_transaction_info, "Purp") | ||
| etree.SubElement(purpose, "Cd").text = line_purpose | ||
| payment_line = line.payment_line_ids[0] |
There was a problem hiding this comment.
Why changing this? Are you sure there's always going to be a payment_line_id?
There was a problem hiding this comment.
It's just the same code that was moved from account_banking_sepa_credit_transfer to account_banking_pain_base, to mutualize the code with account_banking_sepa_direct_debit. On a payment order, an account.payment is always linked to N account.payment.line, and we read the purpose on the account.payment.line because that's where the information is. I didn't change the logic.
| data = { | ||
| "partner_id": partner_id, | ||
| "reference_type": "none", | ||
| "reference_type": "free", |
There was a problem hiding this comment.
I changed the value in the module account_payment_order to have the same value ("free") on account.move and account.payment.line for unstructured communication. Avoids a headache for maintainers. Migration scripts are provided in account_payment_order, cf https://github.com/akretion/banking/blob/16-payment_order-improve_and_cleanup/account_payment_order/migrations/16.0.2.0.0/pre-migration.py#L14
| @openupgrade.migrate() | ||
| def migrate(env, version): | ||
| sct_method = env.ref("account_banking_sepa_direct_debit.sepa_direct_debit") | ||
| sct_method.write({"payment_order_ok": True}) |
|
@alexis-via Any news on this ? |
|
My re-write of the code that generate the address block fixes the bug reported here: |
|
While reading the code, I found a new bug on direct debit, that I later confirmed through tests. I opened a bug report #1336 and I fixed it in my last commit. |
…to filter partner_bank_id on account.move Use native method is_inbound() and is_outbound()
Only copy bank account from move line to payment line for payment orders, not for debit orders.
… linked to debit orders that don't have mandate_required = True
…y check_company=True on mandate_id field
On payment method, check that mandate_required is only be True on inbound methods.
…account.payment.line (same as in account_payment_order)
… to an IBAN account Add python constraint to disallow the creation of a SEPA mandate linked to a non-IBAN account. Fixes bug OCA#1337
|
Hi @alexis-via Regards |
When the user clicks on the button "Generate payment file" in the form view of payment order, it gets a pop-up to save the file.
…nt.payment.order This new field payment_method_code on account.move and account.payment.order is available as invisible in tree+form view: it allows to easily show/hide fields specific to a payment method in format-specific modules. Remove field sepa_payment_method (replaced by generic field payment_method_code) Update field definition of payment_mode_id on account.move: precompute=True and move domain from view to field definition
We still have few clients in v12. Never it's too late. |
If we use a payment order with mandate required, mandate is placed before bank account (more logic) and bank account becomes readonly: it is set by the mandate.
|
Yesterday, I added a small but nice improvement : automatically open download pop-up when clicking on "Generate payment file". |
…fault on payment modes for inbound payment methods with debit orders
|
I made 2 other minors changes to have "good practise" by default when configuring payment modes:
|
|
Hi @alexis-via , Any chance to cherry pick this commit flotho@da2028c to FIX #1193 Moreover, I faced this issue after an upgrade of the pain base module while trying to generate a file :
Any tips to find which state field is concerned ? Regards |
Found, state_id was missing on the partner |
|
Hi @alexis-via , any chance to have a rebase here ? |
NEW FEATURES
SMALL IMPROVEMENTS
SIMPLIFICATION / CODE CLEANUP
TECHNICAL:
FIXES: