[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? |
| 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 |
ghost
mentioned this pull request
|
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 ? |
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 ? |
…1.001.09, not 001.001.03
Add tracking on payment_mode_id on sale.order
alexis-via
force-pushed
the
16-payment_order-improve_and_cleanup
branch
from
c81c37e to
1d9a705
Compare
|
checking the status on this refactor. is the plan still to release for 16.0? |
|
No on my side. This contains too many changes and not split. See my comments at #1174 (review), #1174 (comment), #1174 (comment), etc There's no real intention of this to be merged by the contributor, unless we all accept all the changes he has though on his side and a lot of breaking things. It's a de facto fork. |
|
I had the chance to read over the comments. really we just need to know where to start with an ACH ( USA ) portal feature to allow the payments of statements ( sets of invoices due for the month. ) It would like nice to add rules portal users could apply like the pay on due date or on the 20th of month via bank or set a payment on the due date from the reminder email. looks like payment.acquirer connection and the portal views. We were hoping to build this on 16.0 then migrate to 18 at the same time. |
|
Interesting feature. I think you can mount it on top of the current merged code, as even if this is accepted, it will be on 18, not on this versions that is already almost 3 years old. |
…omplete Fw-ported from OCA/bank-payment 16.0
…es after 36 months Fixes OCA#1477
|
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
github-actions
bot
added
the
stale
13 participants
NEW FEATURES
SMALL IMPROVEMENTS
SIMPLIFICATION / CODE CLEANUP
TECHNICAL:
FIXES: