Skip to content

[13.0][MIG] report_qweb_signer - #522

Merged
OCA-git-bot merged 22 commits into
OCA:13.0from
acsone:13.0-mig-report_qweb_signer-lco
Jul 20, 2021
Merged

OCA-git-bot merged 22 commits into
OCA:13.0from
acsone:13.0-mig-report_qweb_signer-lco

Conversation

@Laurent-Corron

Copy link
Copy Markdown

No description provided.

antespi and others added 21 commits June 29, 2021 10:06
These limits were being hit when printing PDF reports with just 80 pages.
- Good dependency chain
- Context for forcing rendering PDF
- Extra test
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: reporting-engine-12.0/reporting-engine-12.0-report_qweb_signer
Translate-URL: https://translation.odoo-community.org/projects/reporting-engine-12-0/reporting-engine-12-0-report_qweb_signer/
`render_qweb_pdf` must return a tuple of `(content, 'pdf')`
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: reporting-engine-12.0/reporting-engine-12.0-report_qweb_signer
Translate-URL: https://translation.odoo-community.org/projects/reporting-engine-12-0/reporting-engine-12-0-report_qweb_signer/
Fix website on manifest
@Laurent-Corron
Laurent-Corron force-pushed the 13.0-mig-report_qweb_signer-lco branch 3 times, most recently from 39356bb to 89129f2 Compare June 30, 2021 13:33

@sbidoul sbidoul left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good.

res = """<filter name="%s" string="%s"
context="{'group_by':'%s'}"/>""" % (
res = """<filter name="{}" string="{}"
context="{{'group_by':'{}'}}"/>""".format(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change should not be in this PR ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This was changed automatically by pre-commit, I can remove it if you would like ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah ok, it must be pyupgrade that does it. Strange that it changes in this PR and while the main branch is green ?

Comment thread report_qweb_signer/data/defaults.xml Outdated
<record model="ir.config_parameter" id="report_qweb_signer_java_param">
<field name="key">report_qweb_signer.java_parameters</field>
<field name="value">-Xms4M -Xmx4M</field>
<field name="value">-Xms4M -Xmx1g -XX:CompressedClassSpaceSize=256m</field>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this change related to the migration ?

@Laurent-Corron Laurent-Corron Jul 19, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

If I didn't add -XX:CompressedClassSpaceSize=256m to the file, I couldn't run tests (same thing for a gitlab runner)
-Xmx1g was just a test I didn't remove, changed it back to -Xmx4M

@Laurent-Corron
Laurent-Corron force-pushed the 13.0-mig-report_qweb_signer-lco branch from 1917d7f to f9b405a Compare July 19, 2021 13:26
@sbidoul

sbidoul commented Jul 19, 2021

Copy link
Copy Markdown
Member

@pedrobaeza if you are interested. There is also #528 which adds optional endesive support (no java \o/).

@pedrobaeza

Copy link
Copy Markdown
Member

Thanks for the pointer.

If the Java-free solution is working, shouldn't we go directly to that one and don't make it optional?

@sbidoul

sbidoul commented Jul 19, 2021

Copy link
Copy Markdown
Member

We could make endesive the default in 13.0+, and keep the java code path for a little while until people confirm they are happy with endesive.

@pedrobaeza

Copy link
Copy Markdown
Member

If there's no drawback, I think the goal of the module is very clear: to sign the PDF. Letting 2 options for doing exactly the same is maintenance-wise? If there are some differences, then let's put them on the table for deciding.

@sbidoul

sbidoul commented Jul 19, 2021

Copy link
Copy Markdown
Member

I agree we'll need to keep only one at some point. But for now we can't promise that endesive has no drawbacks. It is simpler to deploy and has better performance, but there might be some exotic signature feature missing (such as the LTV option).

@pedrobaeza pedrobaeza added this to the 13.0 milestone Jul 19, 2021

@pedrobaeza pedrobaeza left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK then, let's go with such approach.

Code review

@pedrobaeza

Copy link
Copy Markdown
Member

/ocabot merge nobump

@OCA-git-bot

Copy link
Copy Markdown
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 13.0-ocabot-merge-pr-522-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot
OCA-git-bot merged commit 3b12c11 into OCA:13.0 Jul 20, 2021
@OCA-git-bot

Copy link
Copy Markdown
Contributor

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

@sbidoul
sbidoul deleted the 13.0-mig-report_qweb_signer-lco branch July 20, 2021 08:31
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.