-
-
Notifications
You must be signed in to change notification settings - Fork 800
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][ADD] report_qweb_decimal_precision #842
Conversation
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.
Code LGTM
Feature is a very nice improvement :-)
score = 0 | ||
precision_rec = False | ||
for dp_qweb_rec in dp_qweb_recs: | ||
score_rec = dp_qweb_rec._get_score(record) | ||
if score_rec > score: | ||
precision_rec = dp_qweb_rec |
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.
score = 0 | |
precision_rec = False | |
for dp_qweb_rec in dp_qweb_recs: | |
score_rec = dp_qweb_rec._get_score(record) | |
if score_rec > score: | |
precision_rec = dp_qweb_rec | |
precision_rec = max(dp_qweb_recs, default=None, key=lambda r: r._get_score(record)) |
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.
@len-foss Thanks for the suggestion! Updated the code.
c6a99f7
to
6c57cfe
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.
LGTM.
6c57cfe
to
0da5173
Compare
0da5173
to
f51b1bc
Compare
It worked as expected. |
f51b1bc
to
502ab25
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.
LGTM.
Hello @yostashiro and @AungKoKoLin1997 I can see that both module that you are proposing share similar functionalities. Should I merge this or should we go for #857? I can see that report_qwebt_field_converter is kind of more generic feature. Feedback welcome CC @OCA/reporting-engine-maintainers |
@HviorForgeFlow Indeed, #857 intends to supersede this PR if it works out nicely. We just haven't gotten around to do a review on that PR, which we will follow up in the coming days. |
Ok thanks for your quick feedback, maybe this PR can be closed then. |
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.
Pending superseed #857
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. |
This module allows administrators to define the decimal precision of float fields for QWeb report presentation.
Functionality wise, this basically replaces https://github.com/OCA/reporting-engine/tree/16.0/report_qweb_decimal_place.
Tests need to be added.Configuration:
Result:
@qrtl