Skip to content
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

[11.0][IMP] report_xlsx, make sure print_report_name works #327

Merged

Conversation

kittiu
Copy link
Member

@kittiu kittiu commented Sep 25, 2019

@jffernandez would you help test again? :)

@kittiu kittiu force-pushed the 11.0-imp-report_xlsx-print_report_name branch from 94dd6ad to ec6adb0 Compare September 25, 2019 06:38
Copy link
Member

@jffernandez jffernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's working, but could fail in special cases, look inline comment.
Please, patch the v12 with this too. Sorry for not seeing before.
Thank you!

@@ -29,13 +31,18 @@ def report_routes(self, reportname, docids=None, converter=None, **data):
xlsx = report.with_context(context).render_xlsx(
docids, data=data
)[0]
report_name = report.report_file
if report.print_report_name and not len(docids) > 1:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you should check that docids is set:
if report.print_report_name and docids and not len(docids) > 1:
Just realized that sometimes a report is launched without docids, and it will fail.
In that case is fair to not personalize the name because a main object is not given and the data property would contain the details for rendering the report.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for noticing. I fixed per your request here and also in v12

@kittiu kittiu force-pushed the 11.0-imp-report_xlsx-print_report_name branch from ec6adb0 to 9549996 Compare September 27, 2019 04:40
Copy link
Member

@jffernandez jffernandez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now is safe and working correctly.
Thank you!

@pedrobaeza
Copy link
Member

/ocabot merge patch

@pedrobaeza
Copy link
Member

Is this needed for 12.0?

OCA-git-bot added a commit that referenced this pull request Oct 17, 2019
Signed-off-by pedrobaeza
@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 11.0-ocabot-merge-pr-327-by-pedrobaeza-bump-patch, awaiting test results.

@carlosdauden
Copy link

@kittiu
Copy link
Member Author

kittiu commented Oct 17, 2019

@pedrobaeza
#315
It was merged.

@pedrobaeza
Copy link
Member

Ok, thanks for confirming

@OCA-git-bot OCA-git-bot merged commit 9549996 into OCA:11.0 Oct 17, 2019
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 42bb138. Thanks a lot for contributing to OCA. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants