-
Notifications
You must be signed in to change notification settings - Fork 0
[18.0][MIG] report_py3o #12
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
Conversation
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: reporting-engine-12.0/reporting-engine-12.0-report_xml Translate-URL: https://translation.odoo-community.org/projects/reporting-engine-12-0/reporting-engine-12-0-report_xml/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: reporting-engine-13.0/reporting-engine-13.0-report_xml Translate-URL: https://translation.odoo-community.org/projects/reporting-engine-13-0/reporting-engine-13-0-report_xml/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: reporting-engine-13.0/reporting-engine-13.0-report_xml Translate-URL: https://translation.odoo-community.org/projects/reporting-engine-13-0/reporting-engine-13-0-report_xml/
* No longer need `with Environment.manage()` * Assets are registered in manifest file now * ReportController endpoints no longer take a `token` parameter * The frontend ActionManager has been rewritten as an Owl service
Currently translated at 94.4% (17 of 18 strings) Translation: reporting-engine-15.0/reporting-engine-15.0-report_xml Translate-URL: https://translation.odoo-community.org/projects/reporting-engine-15-0/reporting-engine-15-0-report_xml/ca/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: reporting-engine-16.0/reporting-engine-16.0-report_xml Translate-URL: https://translation.odoo-community.org/projects/reporting-engine-16-0/reporting-engine-16-0-report_xml/
Adds a way to choose the file extension for your XML file, by adding the flexibility of appending more choices. For example, instead of getting `.xml`, this allows to have the report download as `.svg` or even good old `.html` if needed. This is also very useful for some localization purposes, where some scarce software uses uncommon file extensions for their XML files, like `.ffdata` for specific accounting reports here in Lithuania. The change is not breaking, as we set the default to be `.xml` like it was by default anyway.
Currently translated at 100.0% (46 of 46 strings) Translation: reporting-engine-16.0/reporting-engine-16.0-report_py3o Translate-URL: https://translation.odoo-community.org/projects/reporting-engine-16-0/reporting-engine-16-0-report_py3o/it/
Currently translated at 100.0% (46 of 46 strings) Translation: reporting-engine-16.0/reporting-engine-16.0-report_py3o Translate-URL: https://translation.odoo-community.org/projects/reporting-engine-16-0/reporting-engine-16-0-report_py3o/sv/
Currently translated at 100.0% (46 of 46 strings) Translation: reporting-engine-16.0/reporting-engine-16.0-report_py3o Translate-URL: https://translation.odoo-community.org/projects/reporting-engine-16-0/reporting-engine-16-0-report_py3o/it/
f0421f8 to
b965830
Compare
dz0
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.
What was most complicated or needs more review attention?
@dz0 can you just double check [18.0][MIG] report_py3o for any obvious mistakes. |
dz0
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.
So it is ported from v16 directly to v18?
Strange, that no v17 available around...
report_py3o/controllers/main.py
Outdated
| data = list(url_decode(url.split("?")[1]).items()) | ||
| data = list(MultiDict(parse_qs(url.split("?")[1])).items()) |
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.
Why do you use MultiDict here?
it may lose some info if we have query from checkboxes-like form.
example qs: ?a=1&a=2&b=3
>>> md = MultiDict({"a":[1, 2], "b":3})
>>> md
MultiDict([('a', 1), ('a', 2), ('b', 3)])
>>> list(md.items())
[('a', 1), ('b', 3)] # no more a=2 infoThere 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.
without Multidict it would be
>>> list({"a":[1, 2], "b":3}.items())
[('a', [1, 2]), ('b', 3)]also can use parse_qsl variant:
data = parse_qsl(url.split("?")[1])>>> mylist = parse_qsl("a=1&a=2&b=3")
>>> mylist
... [('a', 1), ('a', 2), ('b', 3)]
# If passed to dict, it will leave just **last** value of `a` (where's MultiDict leaves **first**)
>>> dict(mylist)
... {'a': 2, 'b': 3}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 see later data is passed to report_routes
so depends how it will be handled futher -- inside of _render_qweb_[pdf/html/txt/xml]...?
and finally -- will it ever get those multiple param values?
Maybe final-decider would know?
If not clear: would leave at least comment (maybe link to this discussion).
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.
@dz0, this I copied from OCA migration PR and also do not see any comment why it was changed. Although these changes was approved. Will try to check which format will be passed here, but I think will leave this change for now.
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 checked a bit, but did not find out much. But as it is implemented similar way before the change, I think there shouldn't be multiple values for same parameter. Of course one can pass it still, but should expect to to work with multiple.
| _.isUndefined(action.data) || | ||
| _.isNull(action.data) || | ||
| (_.isObject(action.data) && _.isEmpty(action.data)) | ||
| action.data === undefined || | ||
| action.data === null || | ||
| (typeof action.data === "object" && | ||
| Object.keys(action.data).length === 0) |
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.
Here I see was used lodash -- is change needed to get rid of it?
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 also was changed and approved from OCA
| context: JSON.stringify(env.services.user.context), | ||
| context: JSON.stringify(action.context), |
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.
curious: how do you know that it should be action now?
any docs/log of API 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.
This also was copied form orig.
| from PyPDF2.pdf import PageObject | ||
|
|
||
| try: | ||
| # For PyPDF2 <= 1.26.0 |
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 18.0 still rely on olrder PyPDF2? if not, we should just assume latest version.
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.
@oerp-odoo ir depends on python, maybe someone uses older versions: https://github.com/odoo/odoo/blob/18.0/requirements.txt#L57-L58. Or I should leave newer for our needs?
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.
Well Odoo 18 enforces some minimum python, like I think its 3.10. So it might be that it is actually now impossible to use older PyPDF2 at all. Can you check if thats the case? If so, we can drop this check.
Oh I see 3.10 still includes older one:). Then lets keep it as is.
task no. 25641
most of the changes copied from OCA PR, tested locally.