-
-
Notifications
You must be signed in to change notification settings - Fork 709
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
[16.0][IMP] product_pricelist_direct_print add hooks #1782
base: 16.0
Are you sure you want to change the base?
[16.0][IMP] product_pricelist_direct_print add hooks #1782
Conversation
Hi @legalsylvain, @CarlosRoca13, |
6600a87
to
bcf594a
Compare
bcf594a
to
049b53f
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.
LG, always nice to get additional tests
@legalsylvain @CarlosRoca13 |
@@ -95,6 +96,20 @@ def _compute_product_price(self): | |||
else: | |||
self.product_price = price | |||
|
|||
def get_price_for_pricelist(self, pricelist, product): | |||
# TODO enable this instead |
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.
Is it wip ?
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.
More like an open question for reviewers.
Is there a benefit for using a computed field depending on the context rather than using a method?
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 think your right. I'm not sure to understand the current design.
however, changing the code could break possible module overloading this module. Not sure what to think here.
In all cases, if you want to introduce questions, please add better a note in the ROADMAP.rst file that mention "replace computed function xxx by yyy when porting this module". i think it's better than adding commented code with TODO.
What do you think ?
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 agree with @legalsylvain, adding these changes to the module at this stage could cause inheritance issues in other modules that rely on them.
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 definition of the methods is being changed, which will cause errors in modules that use this one.
This pull requests add hooks to the following modules:
This is meant to make overrides of the reports easier.