Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[16.0][IMP] Take allow_out_payment into account on payment order #1340

Open
wants to merge 3 commits into
base: 16.0
Choose a base branch
from

Conversation

lmarion-source
Copy link

@lmarion-source lmarion-source commented Aug 29, 2024

A previous MR existed but only when payment order was triggered from invoices.
We added the case also directly from confirm button on payment order

Original PR: #1177

@lmarion-source lmarion-source force-pushed the 16.0-account_payment_order_allow_out_payment branch 2 times, most recently from 9a523ec to 386628a Compare August 29, 2024 14:05
Copy link
Contributor

@acsonefho acsonefho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GJ 💪
Just a minor change 🙏

account_payment_order/models/account_move.py Outdated Show resolved Hide resolved
@lmarion-source lmarion-source force-pushed the 16.0-account_payment_order_allow_out_payment branch from 386628a to 5f1064e Compare August 30, 2024 06:06
@lmarion-source lmarion-source force-pushed the 16.0-account_payment_order_allow_out_payment branch from 5f1064e to a02bcff Compare September 11, 2024 07:06
@adrienpeiffer
Copy link
Contributor

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 16.0-ocabot-merge-pr-1340-by-adrienpeiffer-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Sep 12, 2024
Signed-off-by adrienpeiffer
@pedrobaeza pedrobaeza added this to the 16.0 milestone Sep 12, 2024
Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have stopped the merge. AFAIK, @sbidoul commented that wants to be able to add the lines, but not to confirm the order, and this is pre-filtering the non-allowed bank accounts previously if I understand the code right.

account_payment_order/models/account_move.py Outdated Show resolved Hide resolved
account_payment_order/models/account_payment_order.py Outdated Show resolved Hide resolved
@OCA-git-bot
Copy link
Contributor

@adrienpeiffer your merge command was aborted due to failed check(s), which you can inspect on this commit of 16.0-ocabot-merge-pr-1340-by-adrienpeiffer-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@adrienpeiffer
Copy link
Contributor

adrienpeiffer commented Sep 12, 2024

@pedrobaeza Not sure to understand. The check is done in draft2open method.

@pedrobaeza
Copy link
Member

I mean this code for example:

https://github.com/OCA/bank-payment/pull/1340/files#diff-bb6441f4ea06462fe1aae1f093f82c4e57460864b29b209676a6c7ce14e49ef9R156

It's avoiding to include the lines in the order.

@adrienpeiffer
Copy link
Contributor

Isn't that exactly the code approved in #1177 ?

@pedrobaeza
Copy link
Member

OK, I based my review in #1177 (comment). Let me check again.

@adrienpeiffer
Copy link
Contributor

@lmarion-source Could you remove blank lines (cfr pedro's review)
@sbidoul If I undersood correctly, you prefer to be able to add lines from invoces even bank account is not validated ?
So basically, we have to remove this line https://github.com/OCA/bank-payment/pull/1340/files#diff-bb6441f4ea06462fe1aae1f093f82c4e57460864b29b209676a6c7ce14e49ef9R158

@sbidoul
Copy link
Member

sbidoul commented Sep 13, 2024

If I undersood correctly, you prefer to be able to add lines from invoces even bank account is not validated ?

It is my impression that Odoo Enterprise works like that, and it makes sense to me, since the process of preparing payment orders is distinct from the process of validating bank accounts, and often done by different persons. So I think it's better to let both processes run in parallel and block at the last moment. Cherry on the cake would be a warning when an non validated bank account is used in a payment line, but that can be done in another PR.

@lmarion-source lmarion-source force-pushed the 16.0-account_payment_order_allow_out_payment branch from a02bcff to 565a13b Compare September 13, 2024 08:10
@gdgellatly
Copy link
Contributor

Hmm, we solved this completely differently. I get the minimal touch here, but we made a seperate module that hit all the places Odoo gets this wrong, mostly purchase order and account move but also payment with a single function on bank to get a valid out payment bank account. Then overrode def _compute_available_partner_bank_ids(self): which for me seems to be the place this belongs

@lmarion-source lmarion-source force-pushed the 16.0-account_payment_order_allow_out_payment branch from 565a13b to 5be3c4b Compare September 26, 2024 08:08
@lmarion-source lmarion-source force-pushed the 16.0-account_payment_order_allow_out_payment branch from 5be3c4b to 1cd2603 Compare September 26, 2024 09:10
    A previous MR existed but only when payment order was triggered from invoices.
    We added the case also directement from confirm button on payment order

    original PR: OCA#1177
@lmarion-source lmarion-source force-pushed the 16.0-account_payment_order_allow_out_payment branch from 1cd2603 to adb3c59 Compare September 26, 2024 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants