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

[13.0] [IMP] report_qweb_signer: allowed reports on certificate #533

Merged

Conversation

ThomasBinsfeld
Copy link
Contributor

Add a new 'Allowed reports' field on the certificate. Only reports listed in this field can be signed. No report means all reports are allowed.

@OCA-git-bot
Copy link
Contributor

Hi @ThomasBinsfeld! 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.

@pedrobaeza
Copy link
Member

pedrobaeza commented Aug 13, 2021

I suppose you need this because not all the reports of the selected model should be signed. Why not taking the occasion to add the whole configuration at report level (if to be signed or not, the domain, etc...)?

@ThomasBinsfeld ThomasBinsfeld force-pushed the 13.0-imp_report_qweb_signer_allowed_reports_tbi branch from 0745a81 to e091dee Compare August 13, 2021 09:28
@ThomasBinsfeld
Copy link
Contributor Author

I suppose you need this because not the reports of the selected model should be signed.

Yes

Why not taking the occasion to add the whole configuration at report level (if to be signed or not, the domain, etc...)?

Not sure there is a real added value to do that. Wyt?

@pedrobaeza
Copy link
Member

Well, we decentralize the configuration per document, and avoid side effects if a new report is added in the model and it gets automatically signed because we haven't excluded it.

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.

Aside from @pedrobaeza suggestion I think is ok 👍

@chienandalu
Copy link
Member

Maybe a comment can be added to the ROADMAP so we can move on with this change.

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 23, 2022
@chienandalu
Copy link
Member

@ThomasBinsfeld I think this is a good addition to the module. Only a ROADMAP map with @pedrobaeza was left, I think. Will you finish it?

@ThomasBinsfeld ThomasBinsfeld force-pushed the 13.0-imp_report_qweb_signer_allowed_reports_tbi branch from e091dee to 04c4d94 Compare October 24, 2022 08:37
@ThomasBinsfeld
Copy link
Contributor Author

ThomasBinsfeld commented Oct 24, 2022

@ThomasBinsfeld I think this is a good addition to the module. Only a ROADMAP map with @pedrobaeza was left, I think. Will you finish it?

Thanks for the review @chienandalu

I rebased the branch and solved the conflicts. This PR is in production for more than a year and is ready to be merged IMO.

I added @pedrobaeza 's refactoring idea in the ROADMAP since i have neither the time nor the intention to implement it.

Add a new 'Allowed reports' field on the certificate. Only reports listed in this field can be signed. No report means all reports are allowed.
@ThomasBinsfeld ThomasBinsfeld force-pushed the 13.0-imp_report_qweb_signer_allowed_reports_tbi branch from 04c4d94 to f047fc4 Compare October 24, 2022 09:08
@pedrobaeza
Copy link
Member

Hi, Thomas, you should add it in readme/ROADMAP.rst, not in README.rst

@ThomasBinsfeld ThomasBinsfeld force-pushed the 13.0-imp_report_qweb_signer_allowed_reports_tbi branch from f047fc4 to 340155f Compare October 24, 2022 10:09
@ThomasBinsfeld
Copy link
Contributor Author

Hi, Thomas, you should add it in readme/ROADMAP.rst, not in README.rst

Fixed, thanks

@pedrobaeza
Copy link
Member

/ocabot merge major

@OCA-git-bot
Copy link
Contributor

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

@OCA-git-bot OCA-git-bot merged commit 32b4ad8 into OCA:13.0 Oct 24, 2022
@OCA-git-bot
Copy link
Contributor

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

@chienandalu
Copy link
Member

Will you forward it to latter versions?

@ThomasBinsfeld ThomasBinsfeld deleted the 13.0-imp_report_qweb_signer_allowed_reports_tbi branch October 24, 2022 11:14
@ThomasBinsfeld
Copy link
Contributor Author

Will you forward it to latter versions?

Here it is #668

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged 🎉 stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants