-
-
Notifications
You must be signed in to change notification settings - Fork 808
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
[ADD] report_generate_helper #827
Conversation
505c4a5
to
d309e48
Compare
def _get_report_converter(self): | ||
return f"_render_{self.report_type.replace('-', '_')}" | ||
|
||
def get_report(self, report_name): |
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.
It would be nice to add a minimal documentation that at least specify that a call to the method will return a tuple filename / content....
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.
LGTM (Code review only)
def _get_report_converter(self): | ||
return f"_render_{self.report_type.replace('-', '_')}" | ||
|
||
def get_report(self, report_name): |
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.
A better name could be generate_report
.... get_report
could mean 'give me the report record for the given report name'
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 intended that this method is public (xml-rpc callable?).
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.
No it's not necessary, I change to _generate_report
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.
Some more documentation would be nice (use case, how to use).
_inherit = "base" | ||
|
||
def _get_report_converter(self): | ||
return f"_render_{self.report_type.replace('-', '_')}" |
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 would add this method to another model that extends ir.action.report
since that is where report_type
is declared. Adding it to a subclass of base
is misleading.
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.
fixed
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
c1900cd
to
475cfef
Compare
475cfef
to
0db02d4
Compare
/ocabot merge nobump |
What a great day to merge this nice PR. Let's do it! |
This PR has the |
Congratulations, your PR was merged at b0b3bc8. Thanks a lot for contributing to OCA. ❤️ |
👋 @marielejeune, the get_report helper is here!