[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. |
chienandalu
left a comment
There was a problem hiding this comment.
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.