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

[FIX] Bug #353 crash when selecting a payment mode that has a variable link to bank accounts #354

Merged
merged 1 commit into from
May 20, 2017

Conversation

alexis-via
Copy link
Contributor

Cf bug #353 for a detailed scenario of the crash.

I you have better ideas to fix this bug, please propose. I suggests this solution because it's the only one I found that currently works on v10.

@@ -162,10 +146,18 @@ def create(self, vals):
@api.onchange('payment_mode_id')
def payment_mode_id_change(self):
journal_id = False
res = {'domain': {
'journal_id': "[('id', '=', False)]",
Copy link
Member

Choose a reason for hiding this comment

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

This should be {'journal_id': []}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I make your modification, then I have access to all journals when no payment mode is selected, which is not what I want : I want the user to select a payement mode first. So I prefer my solution, which works well.

Copy link
Member

Choose a reason for hiding this comment

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

Then please hide the field when no payment mode is selected in the view via attrs attribute.

@sbidoul
Copy link
Member

sbidoul commented Apr 6, 2017

@alexis-via I think the onchange will not trigger when opening a previously saved payment order, so in that case, the domain will be wrong.

Dynamic domains are a pain to get right...

I think it's part of the reasons that pushed @lmignon to create OCA/web#567. You may want to try it.

@alexis-via
Copy link
Contributor Author

@sbidoul Very interesting, thanks !
Another interesting bug report odoo/odoo#16072

@alexis-via
Copy link
Contributor Author

I propose to merge this to avoid the crash. And then, if someone is able to propose a better solution, he will propose another PR.
I don't want to leave an horrible crash like this one too long in an important OCA module.

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.

3 participants