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

[11.0][MIG] account_invoice_update_wizard #109

Open
wants to merge 12 commits into
base: 11.0
Choose a base branch
from

Conversation

grindtildeath
Copy link

Supersedes: #75 (as author repository doesn't exist anymore)

Copy link

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

code review, only on migration + fix commits. LG.

@leemannd
Copy link

Hello @sebastienbeau do you have time to give us a feedback about this?

@rvalyi
Copy link
Member

rvalyi commented Oct 16, 2019

I don't want to anticipate @sebastienbeau response, but sadly Akretion skipped Odoo 11.0 version entirely (it's quite counter productive to try to be expert in every version) so we easily review PRs against 10.0 and 12.0, but for 11.0 it's much less likely...

Copy link

@leemannd leemannd left a comment

Choose a reason for hiding this comment

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

Functional testing only.
@rvalyi I totally undersdand your point. Thanks for the quick answer

@grindtildeath
Copy link
Author

@rvalyi @sebastienbeau If you're using this module in v12.0, you might want to refactor this part:
https://github.com/akretion/odoo-usability/blob/12.0/account_invoice_update_wizard/models/account_invoice.py#L12-L21

This was probably done to avoid having the wizard overwrite the changes done manually when calling its action (as create is called from the UI, the values were overwritten by default_get), but IMO the solution here is cleaner.

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