-
Notifications
You must be signed in to change notification settings - Fork 81
[ADD] util/report: util.add_report #360
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
base: master
Are you sure you want to change the base?
Conversation
cc49acd to
7dae849
Compare
|
upgradeci retry with always only crm |
aj-fuentes
left a comment
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 we can do some simplification here. Some suggestions below. Note that none of the suggestions were tested, take it as a guide.
b40852a to
3525d5d
Compare
3525d5d to
7b3739f
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.
Looks better. Please adapt the documentation to the same format we use elsewhere in this repo. Some suggestions above.
99a9c5c to
c48c6f9
Compare
Add a new function that replaces util.add_to_migration_reports. The new function can automatically parse the provided data as a list of records. It can also limit the number of displayed records to prevent the upgrade report from growing too large. The structure of the note is now defined within the function itself, standardizing its appearance.
c48c6f9 to
64d430b
Compare
| util.report_with_list(summary="The following records were altered.", | ||
| data=cr.fetchall(), | ||
| columns=("id", "name", "city", "comment", "company_id", "company_name") | ||
| row_format="Partner with id {partner_link} works at company {company_link} in {city}, ({comment})", | ||
| links={"company_link": ("res.company", "company_id", "company_name"), "partner_link": ("res.partner", "id", "name")}, | ||
| category="Accounting" | ||
| ) |
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 usual formatting allows for shorter lines. Unless there is some rendering issue?
| util.report_with_list(summary="The following records were altered.", | |
| data=cr.fetchall(), | |
| columns=("id", "name", "city", "comment", "company_id", "company_name") | |
| row_format="Partner with id {partner_link} works at company {company_link} in {city}, ({comment})", | |
| links={"company_link": ("res.company", "company_id", "company_name"), "partner_link": ("res.partner", "id", "name")}, | |
| category="Accounting" | |
| ) | |
| total = cr.rowcount | |
| data = cr.fetchmany(20) | |
| util.report_with_list( | |
| summary="The following records were altered.", | |
| data=data, | |
| columns=("id", "name", "city", "comment", "company_id", "company_name") | |
| row_format="Partner with id {partner_link} works at company {company_link} in {city}, ({comment})", | |
| links={ | |
| "company_link": ("res.company", "company_id", "company_name"), | |
| "partner_link": ("res.partner", "id", "name") | |
| }, | |
| total=total, | |
| category="Accounting" | |
| ) |
EDIT: we can include as well in the example total=...
| """Append the upgrade report with a new entry that displays a list of records. | ||
| The entry consists of a category (title) and a summary (body). | ||
| The entry displays a list of records previously returned by an SQL query, or any list as long as it's passed as a Python List of Tuples. |
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.
Try to keep the lines at max 90 chars (similar as we do for other docstrings here)
| The entry displays a list of records previously returned by an SQL query, or any list as long as it's passed as a Python List of Tuples. | |
| The entry displays a list of records previously returned by an SQL query, or any list | |
| as long as it's passed as a Python List of Tuples. |
| :param list(tuple) data: data to report, each entry would be a row in the report. | ||
| It could be empty, in which case only the summary is rendered. | ||
| :param tuple(str) columns: names for each column in "data", can be referenced in "row_format". | ||
| :param str row_format: the way a row in a list is formatted, using named placeholders, e.g.: | ||
| "Partner {partner_link} that lives in {city} works at company {company_link}." | ||
| :param dict(str, tuple(str, str, str)) links: optional model/record links spec, | ||
| the keys can be referenced in "row_format". | ||
| :param int total: if the original list was limited prior to calling this method, the original, total number of | ||
| records, can be provided with this parameter. | ||
| :param int limit: the maximum number of records that are going to be displayed in the 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 ideas to shorten this and make to the point.
Also use back-quote instead of double quotes to refer to code.
| :param list(tuple) data: data to report, each entry would be a row in the report. | |
| It could be empty, in which case only the summary is rendered. | |
| :param tuple(str) columns: names for each column in "data", can be referenced in "row_format". | |
| :param str row_format: the way a row in a list is formatted, using named placeholders, e.g.: | |
| "Partner {partner_link} that lives in {city} works at company {company_link}." | |
| :param dict(str, tuple(str, str, str)) links: optional model/record links spec, | |
| the keys can be referenced in "row_format". | |
| :param int total: if the original list was limited prior to calling this method, the original, total number of | |
| records, can be provided with this parameter. | |
| :param int limit: the maximum number of records that are going to be displayed in the report. | |
| :param list(tuple) data: data to report, each entry would be a row in the report. | |
| It could be empty, in which case only the summary is rendered. | |
| :param tuple(str) columns: columns in `data`, can be referenced in `row_format`. | |
| :param str row_format: format for rows, can use any name in `columns` or `links`, e.g.: | |
| `"Partner {partner_link} that lives in {city} works at | |
| company {company_link}."` | |
| :param dict(str, tuple(str, str, str)) links: optional model/record links spec, | |
| to use in `row_format`. | |
| :param int total: optional total number of records. | |
| Taken as `len(data)` when `None` is passed. | |
| Useful when `data` was limited by the caller. | |
| :param int limit: maximum number of records to list in the report. | |
| If `data` contains more records the `total` number would be | |
| included in the report as well. |
| row_to_html(columns) # Validate the format is correct, including links | ||
| return report_with_summary(summary=summary, details="", category=category) | ||
|
|
||
| total = total or len(data) |
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.
To avoid weird situation where the number of entries in data is lower than the limit, but we still got total, we should adapt the limit
Do not use the old-style or shortcut, we can be explicit here: None is a special value.
| total = total or len(data) | |
| limit = min(limit, len(data)) | |
| total = len(data) if total is None else total |
| return "<li>{}</li>".format(row_format.format(**row_dict)) | ||
|
|
||
| if not data: | ||
| row_to_html(columns) # Validate the format is correct, including links |
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 will fail. column doesn't have the expected type.
(a.k.a tests are missing)

Add a new function that replaces
util.add_to_migration_reports.The new function can automatically parse the provided data as a list of records. It can also limit the number of displayed records to prevent the upgrade report from growing too large.
The structure of the note is now defined within the function itself, standardizing its appearance.
Example usage
More examples will follow in
odoo/upgrade, replacing the existing use ofutil.add_to_migration_reports.Example 1
Example 2
Example 3
Example 4
Remaining details to discuss
limit.columns(instead of a tuple) or a dictionary of lists forlinks(instead of a dictionary of tuples).add_to_migration_reports.ir.ui.viewrecord corresponding to the note is sometimes incorrect (previously there was the same issue):