-
-
Notifications
You must be signed in to change notification settings - Fork 588
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
[10.0] Improving the mail tracking value #170
Conversation
03df92e
to
5cfe1a5
Compare
@rafaelbn Travis is failing due to an error with Mailgun. The 10.0 branch is failing as well, do you have an idea why? Travis logs of the 10.0 branch (https://travis-ci.org/OCA/social/jobs/234268290#L745-L746):
|
|
||
_inherit = "mail.tracking.value" | ||
|
||
new_value_formated = fields.Char(compute='_compute_formatted_value', |
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.
missing a "t" in fields 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.
Thanks, fixed
def _compute_formatted_value(self): | ||
""" Sets the value formatted field used in the view """ | ||
for record in self: | ||
if record.field_type in ('many2many', 'one2many', 'char'): |
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.
can't we do something better here? Just guessing.... maybe we can use qweb widgets to render values in human readable mode?
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.
According to the type, the values are stored in different fields, we just want to read the value from the correct field. There is no need for a particular rendering more than that.
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.
yes, is not required, I'm just saying that since the formatted value is always a char we can format it using qweb rendering (especially for date, datetime, monetary, etc)
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.
@simahawk ah I get it, indeed it makes sense for date, datetime, monetary, ... Can be a future improvement :)
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.
@TDu can you add a comment here to remind this improvement and add a line in the README roadmap?
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.
Done
5cfe1a5
to
57e3495
Compare
7857fe2
to
b8ec2e4
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.
Tested functionally. I think module's name must be tracking_value_improved
, please change if possible
|
||
To access the new view displaying value changes : | ||
|
||
Settings -> Technical -> Improved tracking values |
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.
Better -> Tracking Values Improved
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.
That's not correct English, Rafa
'name': 'Improved tracking value change', | ||
'version': '10.0.1.0.0', | ||
'summary': 'Improves tracking changes by adding many2many and one2many \ | ||
support. As well as a view to consult changes', |
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.
This description is for user, normal people, they must understand what module makes but they don't know what's a many2many o similar. Please explain it from the functional point of view not a technical one
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.
Additionally there is a bunch of whitespace being added here. The strings should instead be started and stopped.
'my first line. '
'my second line.'
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.
Ok, changed description and removed whitespaces.
# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl.html). | ||
|
||
{ | ||
'name': 'Improved tracking value change', |
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.
IMHO: Tracking value change improved
Please @carlosdauden could you check #170 (comment) thanks! |
=========== | ||
|
||
Bugs are tracked on `GitHub Issues | ||
<https://github.com/OCA/{project_repo}/issues>`_. In case of trouble, please |
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.
Bad link
'name': 'Improved tracking value change', | ||
'version': '10.0.1.0.0', | ||
'summary': 'Improves tracking changes by adding many2many and one2many \ | ||
support. As well as a view to consult changes', |
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.
Additionally there is a bunch of whitespace being added here. The strings should instead be started and stopped.
'my first line. '
'my second line.'
|
||
import json | ||
|
||
from odoo import models, fields, api |
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.
Alphabetize imports
b8ec2e4
to
8b2770b
Compare
Rebase as branch is already green |
4e528c7
to
40796ba
Compare
Change to the readme following comments. |
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.
@TDu can you get back on this? 😄
BTW I triggered runbot rebuild as it was killed
def _compute_formatted_value(self): | ||
""" Sets the value formatted field used in the view """ | ||
for record in self: | ||
if record.field_type in ('many2many', 'one2many', 'char'): |
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.
@TDu can you add a comment here to remind this improvement and add a line in the README roadmap?
I added the comment in the code and readme about improving the rendering depending of value type. |
1668d07
to
e5c9292
Compare
Hello, just a question before merging. Is there any possibility you add some test to improve codecov/patch — 28.57% of diff hit (target 97.13%). ? Please? 🙏 We have been working hard to have high coverage in this repo . Thanks! |
Yes sure, will do. |
2f024c1
to
e8c10bf
Compare
7939834
to
26236f9
Compare
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
26236f9
to
b0c5065
Compare
@rafaelbn Tests are done, coverage three digits number 😁 |
Thank you!!! |
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields ("mail.tracking.value" functionality) by adding many2many and one2many support. As well as a more user friendly view to consult changes recorded.
Improve the tracking of changed values on database fields
("mail.tracking.value" functionality) by adding many2many and one2many support.
As well as a more user friendly view to consult changes recorded.