Allow PDF Forms calculations#1325
Conversation
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
Whats the holdup for this? I'd really appreciate being able to use PDF forms with calculations. |
There was a problem hiding this comment.
Sorry for the long delay and thanks a lot for your contribution!
Due to the delay in the review I took the liberty of addressing myself the points that would have been in the review. I hope that is OK. I added each change as a fixup commit, please see below for the description of each one:
- Adjust the year in the license headers
- Add missing trailing comma
- Remove
AuthorizedAdminSetting; this is only used withIDelegatedSettings, notISettings. As the attribute being set has potential security issues from my point of view for now it is fine to limit it only to the main admins of the instance and it is not strictly needed to implement delegated settings here - Prefer
OCSControlleroverController - Prefer constructor property promotion
- Fix registration of the settings; it seems that you forgot to push some changes, as
IRegistrationContext::registerSettingsdid not exist and therefore the settings were not visible - Remove a parameter that matched its default value
- Remove toast when saving settings (see nextcloud/spreed#16659)
- Adjust text to match setting description with the warning note
- Add specific endpoint for enable scripting rather than a general one for all settings (even if currently there is only one)
- Replace deprecated
checkedwithv-modelinNcCheckboxRadioSwitch
Besides that, I have extracted the fix for handling enableScripting to its own pull request for clarity (as the feature itself was already there, but broken) and because I preferred a slightly different approach.
As the feature in this pull request is adding the UI to configure it rather than the configuration in itself I slightly adjusted the commit message to reflect that.
Finally, I have rebased the pull request on latest master and added an explicit dependency to @nextcloud/vue; this was not needed before, but since the pull request was open the version of the implicit dependency was bumped and it was incompatible with Vue 2.
|
Hey @danxuliu thanks a lot for looking into it. So checking through your review i do not see anything that is missing then or blocking this pr to be merged, no? Then also what about my backport pr to stable32 here #1326 ? |
@nextcloud/vue was already an implicit dependency, but version 9.x was used, which is compatible only with Vue 3. As the PDF viewer uses Vue 2 the explicit dependency needs to be set to version 8.x instead, as otherwise its components would not work (but this does not affect the use of version 9.x by the dependencies themselves). Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
|
Hi @Joly0 , yes, this is almost ready to merge. I wanted to wait until you could take a look to the "review" before squashing the commits in case you wanted to provide any feedback. But as I understand it you are OK with the changes, so I rebased and squashed them. There is a little missing thing, though. As the main author of the (now second) commit, could you amend it to add the Developer Certificate of Origin? You can see how to do it here: https://github.com/nextcloud/files_pdfviewer/pull/1325/checks?check_run_id=68581080214 Please remember to fetch/pull the latest changes first, as otherwise you could overwrite them :-) Then I will add the built JavaScript files and the pull request will be ready to merge :-) Regarding the backport to stable32 I will take care of it once this one is ready (and merges to stable32 are possible again, as right now merges are blocked due to an incoming release). |
9d1bd75 to
4007fd0
Compare
This PR adds a UI to configure the pdf viewer to allow calculations (secure in a sandbox). Can be enabled/disabled through the admin settings globally, default is disabled. Uses pdf.js´s sandbox feature, which should make this as secure as it can get. Fixes nextcloud#1265 Signed-off-by: Joly0 <13993216+Joly0@users.noreply.github.com>
|
Hey @danxuliu thanks for the information. I added the DCO (hopefully correctly) to the commit. I had some problems with my gpg signing (new laptop didnt have the signing key yet, ups 😅), but it should all be correct now. |
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
|
@Joly0 Perfect! For the record, only the I have added the built JavaScript files (as it is a shipped app they need to be included in every pull request that changes them) and added a license file used by one of the new (sub-)dependencies (due to the explicit dependency to nextcloud/vue 8.37.0). The psalm failure will be addressed in a different pull request, as it seems that the new code triggers a bug in psalm itself when used with PHP 8.5 that was fixed in psalm 6.14.1. As mentioned I will take care of the backports to stable33 and stable32 (note that in general we do not backport features, but we will do an exception in this case) once the maintenance releases are published and merges in the stable branches are allowed again. Thanks for your contribution and sorry again for the delay. |
There was a problem hiding this comment.
Tested and works 👍
psalm failure is a bug in psalm itself and fixed in a newer release. This will be addressed in its own pull request.
|
/backport to stable33 please |
|
/backport to stable32 please |
This PR configures the pdf viewer to allow calculations (secure in a sandbox). Can be enabled/disabled through the admin settings globally, default is disabled. Uses pdf.js´s sandbox feature, which should make this as secure as it can get.
Fixes #1265