-
Notifications
You must be signed in to change notification settings - Fork 30.4k
[FIX] account : compute invoice_partner_bank_id when computing bank_partner_id #92660
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
[FIX] account : compute invoice_partner_bank_id when computing bank_partner_id #92660
Conversation
agr-odoo
left a comment
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.
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.
To avoid multicompany issue you could should add a check on the bank company_id
move.bank_partner_id.filtered(lambda bank: bank.company_id.id in (False, move.company_id.id))
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.
@agr-odoo I updated the commit, thank you 🙏
5a198fd to
f288669
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.
There is a problem here as I am not usre invoice_partner_bank_id is a computed field and you do not depend on the company_id/journal_id in the api.depends. @abla001
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.
Indeed, because these are 2 stored computed fields, you need to compute them in two different functions
william-andre
left a comment
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.
Like seen in this PR #85982, the field was wrongly "manually" computed at some places, instead of computing it correctly in those places, you should remove the fact that it is set explicitly to a possible wrong value.
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.
- Don't compute fields that are not supposed to be computed
- Don't change it if it was already a valid bank
- You can also use
filtred_bank_partner_id.bank_ids[:1]to take the first record safely - filtered
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.
Indeed, because these are 2 stored computed fields, you need to compute them in two different functions
|
Also, the commit message can be more explicit on who the invoice is directed at . To a customer or a vendor? Because these have opposite behaviors in the matter you are fixing. |
f288669 to
6a7542b
Compare
|
@jco-odoo, @william-andre, thank you so much for the feedback, I updated the commit 🙏 |
49ae196 to
4d827cb
Compare
william-andre
left a comment
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.
Runbot is red, please also check account.payment.register._get_line_batch_key (#85982)
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.
Lets keep related arguments together, and lets reduce the amount of lines modified
| invoice_partner_bank_id = fields.Many2one('res.partner.bank', string='Bank Account', store=True, readonly=False, | |
| compute="_compute_invoice_partner_bank_id", | |
| invoice_partner_bank_id = fields.Many2one('res.partner.bank', string='Bank Account', | |
| compute="_compute_invoice_partner_bank_id", store=True, readonly=False, |
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.
| filtred_bank_partner_id = move.bank_partner_id.filtered(lambda bank: bank.company_id.id in (False, move.company_id.id)) | |
| move.invoice_partner_bank_id = filtred_bank_partner_id.bank_ids[:1] | |
| filtered_bank_partner_id = move.bank_partner_id.filtered(lambda bank: bank.company_id.id in (False, move.company_id.id)) | |
| move.invoice_partner_bank_id = filtered_bank_partner_id.bank_ids[:1] |
|
for current red runbot, this is happening because we are writing on odoo/addons/account/models/account_move.py Line 2590 in 550f9e7
but lines are also written at the same time and they will trigger a recompute of an example of a fix (a little hacky) would be to do: + invoice.invoice_partner_bank_id # just used so the field is in cache when recomputed before it is written
move.write({'invoice_line_ids' : [(5, 0, 0)], 'invoice_partner_bank_id': False}) |
4d827cb to
ec9a347
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.
I'm not sure what triggers the recompute
invoice_partner_bank_id is triggered by bank_partner_id
And bank_partner_id is triggered by commercial_partner_id,
Then commercial_partner_id is triggered by partner_id, which doesn't depend on invoice_line_ids or invoice_partner_bank_id
If the issue is that the field is protected because it is recomputed, wouldn't the solution be to not set the field to False in the first place?
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.
setting invoice_partner_bank_id to False was added in this fix : 564c4f5
I'm afraid that removing breaks that fix.
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 field is protected because we are writing on it:
Lines 3559 to 3570 in 550f9e7
| if field.inverse or (field.compute and not field.readonly): | |
| if field.store or field.type not in ('one2many', 'many2many'): | |
| # Protect the field from being recomputed while being | |
| # inversed. In the case of non-stored x2many fields, the | |
| # field's value may contain unexpeced new records (created | |
| # by command 0). Those new records are necessary for | |
| # inversing the field, but should no longer appear if the | |
| # field is recomputed afterwards. Not protecting the field | |
| # will automatically invalidate the field from the cache, | |
| # forcing its value to be recomputed once dependencies are | |
| # up-to-date. | |
| protected.update(self._field_computed.get(field, [field])) |
but before we do the field write on invoice_partner_bank_id, we do the field write on 'line_ids' that is causing an account.move.line().unlink() that is causing a flush of everything and a recompute of everything.
edit: another solution (without ORM change) would be to do .invoice_partner_bank_id = False after writing on the lines, from the linked commit this seemed to be the intention.
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.
I'm afraid that removing breaks that fix.
It wouldn't break the fix if the compute function is called again...
the field is protected because we are writing on it:
Yes, that I know
that is causing a flush of everything and a recompute of everything.
AFAIK, stored fields should not be recomputed unless triggered by a field change. I don't see what would trigger it.
My understanding with your hack is that indeed you cache the old value, but that value is the wrong one, and we would get back to the behavior before 564c4f5
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.
@william-andre , I applied your suggestion, I tested and you were right, removing False doesn't break the old fix, thank you 🙏
d5d2b2c to
c07f356
Compare
william-andre
left a comment
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.
Final touch to be sure
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 you add the type in here to be sure that the bank_partner_id is recomputed here
https://github.com/odoo/odoo/blob/c07f3560d7b9c23b6a2bbb74363670e517c75951/addons/account/models/account_move.py#L2581
To ensure that invoice_partner_bank_id is recomputed too
…artner_id To reproduce ============ - add a bank account to *MyCompany* - create contact who has a bank account - create customer invoice for this contact - create credit note from this invoice the Bank Account of the credit note is set to *MyCompany's* bank account, where it should be the customer's account Purpose ======= when creating the credit note we don't set the field `invoice_partner_bank_id` Specification ============= to solve the issue we compute the field `invoice_partner_bank_id` when computing the field `bank_partner_id` opw-2862601
c07f356 to
f4bf833
Compare
|
@william-andre, I applied your suggestion, thank you and @nle-odoo for the follow-up 🙏 |
william-andre
left a comment
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.
@robodoo r+
…artner_id To reproduce ============ - add a bank account to *MyCompany* - create contact who has a bank account - create customer invoice for this contact - create credit note from this invoice the Bank Account of the credit note is set to *MyCompany's* bank account, where it should be the customer's account Purpose ======= when creating the credit note we don't set the field `invoice_partner_bank_id` Specification ============= to solve the issue we compute the field `invoice_partner_bank_id` when computing the field `bank_partner_id` opw-2862601 closes #92660 Signed-off-by: William André (wan) <[email protected]>
|
This pull request has forward-port PRs awaiting action (not merged or closed): #93467 |
4 similar comments
|
This pull request has forward-port PRs awaiting action (not merged or closed): #93467 |
|
This pull request has forward-port PRs awaiting action (not merged or closed): #93467 |
|
This pull request has forward-port PRs awaiting action (not merged or closed): #93467 |
|
This pull request has forward-port PRs awaiting action (not merged or closed): #93467 |
|
This pull request has forward-port PRs awaiting action (not merged or closed): #94152 |
2 similar comments
|
This pull request has forward-port PRs awaiting action (not merged or closed): #94152 |
|
This pull request has forward-port PRs awaiting action (not merged or closed): #94152 |
|
This pull request has forward-port PRs awaiting action (not merged or closed): #94152 |
…artner_id To reproduce ============ - add a bank account to *MyCompany* - create contact who has a bank account - create customer invoice for this contact - create credit note from this invoice the Bank Account of the credit note is set to *MyCompany's* bank account, where it should be the customer's account Purpose ======= when creating the credit note we don't set the field `invoice_partner_bank_id` Specification ============= to solve the issue we compute the field `invoice_partner_bank_id` when computing the field `bank_partner_id` opw-2862601 closes odoo#92660 Signed-off-by: William André (wan) <[email protected]>
|
This pull request has forward-port PRs awaiting action (not merged or closed): #94152 |
…artner_id To reproduce ============ - add a bank account to *MyCompany* - create contact who has a bank account - create customer invoice for this contact - create credit note from this invoice the Bank Account of the credit note is set to *MyCompany's* bank account, where it should be the customer's account Purpose ======= when creating the credit note we don't set the field `invoice_partner_bank_id` Specification ============= to solve the issue we compute the field `invoice_partner_bank_id` when computing the field `bank_partner_id` opw-2862601 closes odoo/odoo#92660 Signed-off-by: William André (wan) <[email protected]>

To reproduce
the Bank Account of the credit note is set to MyCompany's bank account, where it should be
the customer's account
Purpose
when creating the credit note we don't set the field
invoice_partner_bank_idSpecification
to solve the issue we compute the field
invoice_partner_bank_idwhen computing the fieldbank_partner_idopw-2862601