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

[14.0][MIG] report_qweb_signer #534

Merged
merged 24 commits into from
Feb 21, 2022

Conversation

omar7r
Copy link
Contributor

@omar7r omar7r commented Aug 30, 2021

No description provided.

antespi and others added 18 commits August 30, 2021 13:24
OCA Transbot updated translations from Transifex
OCA Transbot updated translations from Transifex
These limits were being hit when printing PDF reports with just 80 pages.

OCA Transbot updated translations from Transifex

[UPD] Update report_qweb_signer.pot
- Good dependency chain
- Context for forcing rendering PDF
- Extra test

[UPD] README.rst

[UPD] Update report_qweb_signer.pot

Update translation files

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/

[UPD] README.rst
`render_qweb_pdf` must return a tuple of `(content, 'pdf')`
Update translation files

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
[UPD] Update report_qweb_signer.pot

[UPD] README.rst
Update translation files

Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: reporting-engine-13.0/reporting-engine-13.0-report_qweb_signer
Translate-URL: https://translation.odoo-community.org/projects/reporting-engine-13-0/reporting-engine-13-0-report_qweb_signer/
@OCA-git-bot
Copy link
Contributor

Hi @omar7r! Thank you very much for this contribution. As the addon you are improving does not have a declared maintainer, I take the opportunity to mention that you can consider adopting it. To do so, please read the maintainer role description, and, if interested, create a pull request to add your GitHub login to the maintainers key of the addon manifest.

@omar7r omar7r force-pushed the 14.0-mig-report_qweb_signer branch from 206f3b4 to 54bf4b0 Compare August 30, 2021 19:01
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Hi @omar7r please include #542

@pedrobaeza
Copy link
Member

There has been also some moves in v13 version to add (and planning to switch to) a new Python library.

@chienandalu
Copy link
Member

There has been also some moves in v13 version to add (and planning to switch to) a new Python library.

At the time I didn't find a library mature enough to stick to, although the current implementation isn't very neat anyway...

@pedrobaeza
Copy link
Member

Please see #528. It seems this one is promising. @sbidoul how were your tests? Should we remove java library in favor of this one?

@sbidoul
Copy link
Member

sbidoul commented Oct 1, 2021

@pedrobaeza @chienandalu We have positive results so far with endesive but no large scale production feedback yet. So its probably premature to drop the java code yet.

@chienandalu
Copy link
Member

Oh, yeah. That the one I had spotted but two years ago it was fairly new

@pedrobaeza
Copy link
Member

Ok, then at least this PR should be updated with latest 13.0 commits

@sbidoul
Copy link
Member

sbidoul commented Oct 1, 2021

And there is also #533 that is pending.

We don't want our custom parameters be reset every time the module
updates.

TT32256
@pedrobaeza
Copy link
Member

@omar7r did you check about the new sign method? Travis is red by the way.

@omar7r
Copy link
Contributor Author

omar7r commented Nov 16, 2021

Yes, I tested two options, both works fine. I think that Travis fails because needs to have installed the package swig in the system, where can I add it?

@pedrobaeza
Copy link
Member

You can put it on requirements.txt file in the root folder.

@pedrobaeza
Copy link
Member

Should we remove Java library then and leave only the Pythonic one?

@chienandalu
Copy link
Member

Out of curiosity, how do both methods compare in performance when printing a big number of signed documents?

@omar7r
Copy link
Contributor Author

omar7r commented Nov 16, 2021

Swig it's a dependency in the machine not at python level.

In our case, we are using java's library in production, because the project was a migration, from 10.0 to 14.0, I don't test the python library printing several invoices at time. I don't know if someone use the pythonic lib in production and could give feedback, if not, it will be a good test to do, before the decision.

@pedrobaeza
Copy link
Member

Then you need to change .travis.yml to install such dependency:

https://github.com/OCA/reporting-engine/blob/13.0/.travis.yml#L15

Copy link
Member

@ernestotejeda ernestotejeda left a comment

Choose a reason for hiding this comment

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

On runbot:
Peek 07-02-2022 17-09

@ernestotejeda
Copy link
Member

Hello @omar7r , are you working on this PR? If not, I can supersede it to speed up the migration, thanks.

@omar7r
Copy link
Contributor Author

omar7r commented Feb 16, 2022

Hi @ernestotejeda

Yes, but I didn't have much time since your comment, on friday I think I can review it.

@omar7r
Copy link
Contributor Author

omar7r commented Feb 18, 2022

@ernestotejeda I tested it again locally and it works, maybe Runbot doesn't have enough memory for running Java.

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Ok, perfect :) Can you squash the migration commits?

Copy link
Member

@ernestotejeda ernestotejeda left a comment

Choose a reason for hiding this comment

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

tested locally 👍🏼

@pedrobaeza
Copy link
Member

/ocabot migration report_qweb_signer
/ocabot merge nobump

@OCA-git-bot OCA-git-bot added this to the 14.0 milestone Feb 21, 2022
@OCA-git-bot OCA-git-bot mentioned this pull request Feb 21, 2022
14 tasks
@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 341c75b into OCA:14.0 Feb 21, 2022
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 945e958. 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.