-
-
Notifications
You must be signed in to change notification settings - Fork 808
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
[13.0] [IMP] report_qweb_signer: allowed reports on certificate #533
Conversation
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 |
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...)? |
0745a81
to
e091dee
Compare
Yes
Not sure there is a real added value to do that. Wyt? |
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. |
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.
Aside from @pedrobaeza suggestion I think is ok 👍
Maybe a comment can be added to the ROADMAP so we can move on with this change. |
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. |
@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? |
e091dee
to
04c4d94
Compare
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.
04c4d94
to
f047fc4
Compare
Hi, Thomas, you should add it in readme/ROADMAP.rst, not in README.rst |
f047fc4
to
340155f
Compare
Fixed, thanks |
/ocabot merge major |
This PR looks fantastic, let's merge it! |
Congratulations, your PR was merged at e9ee16b. Thanks a lot for contributing to OCA. ❤️ |
Will you forward it to latter versions? |
Here it is #668 |
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.