-
-
Notifications
You must be signed in to change notification settings - Fork 414
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
[12.0] [REF] Account payment return: invoice returned vals #310
[12.0] [REF] Account payment return: invoice returned vals #310
Conversation
7270724
to
df567a2
Compare
@@ -228,8 +227,10 @@ def action_confirm(self): | |||
# returned_moves: debit on customer account (from invoice move) | |||
returned_moves = move_line.matched_debit_ids.mapped( | |||
'debit_move_id') | |||
returned_moves.mapped('invoice_id')._payment_returned( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this return more than one invoice and thus give singleton error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may return more than one invoice when a payment reconciled with several invoices is returned.
_payment_returned
is api.multi, it should not be problematic.
6473e58
to
630a0c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _payment_returned
event method is now on account.move.line
which is more flexible.
Still LGTM.
@pedrobaeza ok for you? This small refactoring is the basis for the new features in the other PRs.
Seems a bit weird that you have the trigger but call a prepare method in other model. Also there seems to be now 2 |
Indeed it's a bit weird but we kept the method on Moreover, some data useful for the computation of invoice returned vals are stored on |
@pedrobaeza to me it looks clean. You can see an example usage this PR enables in cf983bf |
I think we should keep then both |
@pedrobaeza I don't understand: |
Yeah, sorry, I mixed things as I was seeing only one commit diff. Please squash both commits in one and I'll merge. |
630a0c2
to
f88699b
Compare
@pedrobaeza Commits squashed. |
/ocabot merge patch |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at 2577c02. Thanks a lot for contributing to OCA. ❤️ |
For better extensibility, notify the invoice that a payment return has arrived and let it do what it implies for itself.