Skip to content

Conversation

@aritzolea
Copy link

@aritzolea aritzolea commented Jan 31, 2024

Payments (account.payment) belonging to a payment order (account.payment.order) are assigned the partner bank account on their creation which doesn't follow the pattern defined on the _compute_available_partner_bank_ids from the account module.

This is not important while the compute method is not triggered, but if it gets triggered the original partner_bank_id value gets replaced by the value defined in the compute method (journal's bank in case of incoming payment orders). This affects to the creation of sepa files, because the bank accounts listed in the file are not correct.

This fix overrides _compute_available_partner_bank_ids function on account_payment_order module. For payments which belong to a payment order and are generated from payment lines (account.payment.line), it assigns the partner_bank_id from the origin payment lines.

Copy link

@gerard-vacas gerard-vacas left a comment

Choose a reason for hiding this comment

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

LGTM

@aritzolea aritzolea changed the title [WIP][16.0][FIX] account_payment_order: Keep partner_bank_id correct value on payments belonging to payment order [16.0][FIX] account_payment_order: Keep partner_bank_id correct value on payments belonging to payment order Feb 1, 2024
@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza pedrobaeza added this to the 16.0 milestone Feb 7, 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 don't really see the use case for the re-triggering you are talking about, and the tests are just launching manually the compute method.

@aritzolea aritzolea force-pushed the 16.0-fix-account_payment_order-compute_available_partner_banks branch from d5694f6 to c45d43e Compare February 8, 2024 12:35
# fields value doesn't change
with Form(payment) as payment_form:
payment_form.partner_id = self.env["res.partner"]
payment_form.partner_id = payment_line.partner_id
Copy link
Member

Choose a reason for hiding this comment

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

This test case is still a non real one and doesn't reflect a business case. What are you trying to accomplish? Maybe what we have to do is to forbid changing account.payment linked to payment orders.

Copy link
Author

Choose a reason for hiding this comment

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

Modified tests to a more "likely" use case un which a user can accidentaly modify a field on account.payment model's form view. partner_bank_id field would get an incorrect value.

I recorded a video reproducing the issue. In this case field turns to False because journal has no bank:

Odoo.-.PAY0001.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @pedrobaeza is it ok?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the video. I think the proper fix should be to block any change (through an override in the write method of the account.payment) if it's linked to a payment order.

@aritzolea aritzolea force-pushed the 16.0-fix-account_payment_order-compute_available_partner_banks branch from c45d43e to 7d2ad66 Compare February 15, 2024 15:09
@aritzolea aritzolea force-pushed the 16.0-fix-account_payment_order-compute_available_partner_banks branch 2 times, most recently from 2468389 to d825924 Compare April 24, 2024 07:56
@bencoronel bencoronel force-pushed the 16.0-fix-account_payment_order-compute_available_partner_banks branch from d825924 to e8670c1 Compare July 2, 2024 05:04
…en compute method is triggered

When account module's _compute_available_partner_bank_ids method was executed over payments belonging to a payment order an incorrect value for partner_bank_id field was set.
This module now overrides _compute_available_partner_bank_ids function so that payments belonging to a payment order keep their partner_bank_id value
@aritzolea aritzolea force-pushed the 16.0-fix-account_payment_order-compute_available_partner_banks branch from e8670c1 to c11242d Compare September 13, 2024 07:32
@pedrobaeza
Copy link
Member

This is better fixed IMO here: #1346

Do you have anyway the trigger conditions?

@aritzolea
Copy link
Author

This is better fixed IMO here: #1346

Do you have anyway the trigger conditions?

A trigger condicion can be seen in the video I recorded for this PR a few months ago

@pedrobaeza
Copy link
Member

But I wasn't able to reproduce it on the same conditions, @aritzolea. It seems something that depends on the module stack. Did you do it on runboat? Anyway, as said, the other PR is more focused IMO.

@pedrobaeza
Copy link
Member

Do you agree then to switch to the other PR?

@aritzolea
Copy link
Author

But I wasn't able to reproduce it on the same conditions, @aritzolea. It seems something that depends on the module stack. Did you do it on runboat? Anyway, as said, the other PR is more focused IMO.

I was able to reproduce the error in 16.0 runboat, and the solution in your PR's runboat

@pedrobaeza
Copy link
Member

OK, then are you ok with continuing with the other PR?

@aritzolea
Copy link
Author

OK, then are you ok with continuing with the other PR?

Yes, it can be continued on the other PR.

@pedrobaeza
Copy link
Member

Thanks.

@pedrobaeza pedrobaeza closed this Sep 16, 2024
@aritzolea aritzolea deleted the 16.0-fix-account_payment_order-compute_available_partner_banks branch October 11, 2024 07:51
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.

5 participants