-
-
Notifications
You must be signed in to change notification settings - Fork 790
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
report_xlsx: fix report context propagation on records #259
Conversation
digging into this issue turns out that if you don't pass |
6b2f52f
to
da3e7ea
Compare
@simahawk in mis builder 10, we have this https://github.com/OCA/mis-builder/blob/be28ba9de4c67bbc4339522bdb1686b4e942ada3/mis_builder/models/mis_report_instance.py#L618-L627 and it works fine. |
@sbidoul but that code has been migrated already to v11 since there's not return {
'name': 'MIS report instance XLSX report',
'model': 'mis.report.instance',
'type': 'ir.actions.report',
'report_name': 'mis_builder.mis_report_instance_xlsx',
'report_type': 'xlsx',
'report_file': 'mis_report_instance',
'context': context,
} and this does not work. [...]
'context': context,
'data': {'__make_report_controller_work__': 1}, by adding |
@simahawk is this still WIP? |
@sbidoul I'm not working on this since ages 😅 |
All I can say is the fake data dict is needed to propagate the context for pdf reports too. So at least the hack is consistent ;) Why it is so is beyond me... |
It's because the controller relies on the automatic conversion of the params in the request and if the param is not in the request they ignore it completely 🤦♂️ |
ok then I'll update this PR w/ a mention in the docs |
@simahawk I just debugged a bit, and I think the context is actually lost here: reporting-engine/report_xlsx/models/ir_report.py Lines 19 to 21 in 9eb9a42
So replacing that with |
7aac6bd
to
706bdf2
Compare
I removed context manipulation and fixed up the places where the context was replaced instead of updated. I cannot test it now, I'll see if someone else can. |
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 that to fix the context propagation only is needed one change, with that it works.
See inline comments.
Thank you for your PR!
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 if you'd have 2 minutes to revert the **context
changes which are not really necessary?
For the rest I tested and the fix works fine. Thanks!
706bdf2
to
4a1398c
Compare
I changed "blindly" what have you asked for, no time to test, sorry guys :/ |
Thanks! /ocabot merge patch fixes #326 |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at 532e84e. Thanks a lot for contributing to OCA. ❤️ |
Without this change If you return a custom action that sets a custom context -> you lose it -> if your report depends on it... it's broken 😭
@adrienpeiffer @sbidoul this fixes XLS export on
mis.report.instance
😉Fixes #326