forms

forms copied to clipboard

This PR introduces possibility add new File Question type:

This is how it works under the hood:

1. when user select file during form fill - we're uploading it to form owner's folder forms/unsubmitted/{formId}. {formName}/{questionId}. {questionName} and store inside oc_forms_v2_uploaded_files table.
2. once form submitted we are moving file to forms/{formId}. {formName}/{submissionId}/{questionId}. {questionName}, store original file name as oc_forms_v2_answers.text and reference to uploaded file as oc_forms_v2_answers.file_id. And remove row from oc_forms_v2_uploaded_files

There are validations for:

• file mime types/extensions
• max file size
• max files count

Apr 01 '24 23:04 Koc

Nice for starting implementing this!

when user select file during form fill - we're uploading it to form owner's folder (forms/form-{formId}) and store inside oc_forms_v2_uploaded_files table. To prevent name duplication we are using uuid as file name

I am not sure why we need a new table? Would it be not sufficent to just store the filename in the answers? As everything else is already stored in Nextcloud's file database.

But I am also not the fond of having a custom API for something Nextcloud provides out-of-the-box: Files. While using Nextcloud's webdav would be pretty simple for this, I am not that sure how to do authentication there - so without any research a custom endpoint is currently probably easier...

Apr 02 '24 10:04 susnux

While using Nextcloud's webdav would be pretty simple for this, I am not that sure how to do authentication there - so without any research a custom endpoint is currently probably easier...

What I could imagine is creating shares for the forms folders with upload only permission (or upload+view when having form-results permissions). Then one could directly upload to that folder using webdav.

Apr 02 '24 13:04 susnux

Open questions:

• do we need support of multiple files?
  • if yes - do we need configure max amount of files?
• do we need add validation for max file size?
• do we need add validation for supported mime types?
  • if yes - how it should be configured from UX perspective?

I think that all of those make sense. That's also how Google Forms does it:

Available file types:

The settings could be stored into our extraSettings

Apr 02 '24 16:04 Chartman123

Mates, I've see one problem with multiple files: there is no possibility to set multiple hyperlinks inside single cell. see https://answers.microsoft.com/en-us/msoffice/forum/all/multiple-hyperlinks-in-one-cell-excel/7af82648-b7a0-4c9c-b4d7-c485b256dc06 for the reference. Even if it possible with LibreOffice Calc, PhpSpreadsheet does not support this.

Any ideas how to workaround that?

Apr 02 '24 23:04 Koc

I am not sure why we need a new table? Would it be not sufficent to just store the filename in the answers? As everything else is already stored in Nextcloud's file database.

@susnux the problem is that we need upload file and store it id somewhere before form submission. Of course, I can use only \OCP\Files\Node::getId but:

• it's not safe from security perspective. User can provide arbitrary fileId during form submission as fileId is simple integer. Uuid fixes this problem
• we want have original user's file name
• would be nice to cleanup uploaded files for unsubmitted forms after some delay

Apr 02 '24 23:04 Koc

Any ideas how to workaround that?

What about creating a subfolder per submission and then just linking the folder instead of the single files?

Apr 03 '24 05:04 Chartman123

Can anybody help me with styles? :pray:

Apr 05 '24 23:04 Koc

Can anybody help me with styles? 🙏

pinging @jancborchardt Do you have a preferred way to do this design wise?

I think we should at least have it in submenus like for the file linking. Another possibility would be a NcModal...

Apr 05 '24 23:04 Chartman123

• it's not safe from security perspective. User can provide arbitrary fileId during form submission as fileId is simple integer. Uuid fixes this problem

Why should the UUID be safe? It is just obfuscation. If you see a security issue here please inform me, but for me it should be pretty ok to have it by fileid, we just need to check it is located under the owner's forms/{form}.

• we want have original user's file name

This one would be preserved, there is no need to rename the file at all, see the suggestion of subdirs for submissions.

• would be nice to cleanup uploaded files for unsubmitted forms after some delay

Would also be possible.

Given {user}/{formsfolder}/{formid} we can create a folder {submissionid}, this folder holds all submission files.

The process could be:

1. Upload the files ({user}/{formsfolder}/{formid}/{some temporary name})
2. Submit the form
3. The temporary files are renamed into the submission id (4.) If not submitted a background job removes all temporary files

Apr 08 '24 11:04 susnux

One question is of course where to store it, there are two options (I see):

1. In user folder directly - Then the files count into the user quota.
2. In app data like collectives is doing and just provide a mount point - Then the files do not count into the user quota.

Apr 08 '24 12:04 susnux

2. In app data like collectives is doing and just provide a mount point - Then the files do not count into the user quota.

I'd prefer app data

Apr 08 '24 12:04 Chartman123

@susnux

Why should the UUID be safe? It is just obfuscation. If you see a security issue here please inform me,

As fileId is a just integer - I can brutforce them and send fileId of File which was uploaded by another person to insertSubmission

@Chartman123 can you suggest how can I get this app folder? For now I'm using $userFolder = $this->storage->getUserFolder($form->getOwnerId());. What should I use for app folder?

Apr 12 '24 10:04 Koc

@Koc I don't know much about that, but I found this in the dev docs: https://docs.nextcloud.com/server/stable/developer_manual/basics/storage/appdata.html

Apr 12 '24 10:04 Chartman123

I'd prefer app data

BTW this could be exploited by create own forms and upload files there to create files even if the quota is exceeded. Not sure if this would be a problem.

An other problem with app data would be that it does not allow any integration out of the box, e.g. someone submits a file -> you can only access it within the forms app with custom handling. But you could not integrate it in workflows or use it in other apps. To enable this integration we would need a mount of the appdata, like collectives is doing.

Apr 12 '24 22:04 susnux

But I also think app data is probably the best location as that data is not directly connected to an user (because in the future a form might be collaborative). And we might want to also use it in the future for e.g. form attachments (embedded images or similar).

Apr 12 '24 22:04 susnux

To enable this integration we would need a mount of the appdata, like collectives is doing.

Yes, that's what I meant, use app data folder and mountpoint Forms in root folder

Apr 13 '24 07:04 Chartman123

Mates, I've updated PR description with new screenshoots. Can we keep UI design as is for the first iteration?

I will try to use app folder, but it requires also adding mount. I have no experience in that :worried:

Apr 16 '24 23:04 Koc

Can we keep UI design as is for the first iteration?

What about putting the menu entries into two submenus? Otherwise this menu gets very long...

Apr 17 '24 06:04 Chartman123

sure, I can try to do that. Any ideas how exactly split that? e.g. something like:

[ ] Required checkbox
<NcActionLink>Allowed all file types</NcActionLink> -> click opens submenu submenu, once file types are selected, label should be changed to `Allowed file types: documents, images`

but what labels use for files size/count?

Apr 17 '24 08:04 Koc

I've updated UI for file types management. Please check

Apr 23 '24 23:04 Koc

I've updated UI for file types management

Yes, that's better :)

Apr 24 '24 06:04 Chartman123

I've added a cron job that will cleanup files that were uploaded more than 2 days ago but still not submitted. And also there is a validation that file is exists during submission, here an example of the error message:

Apr 25 '24 23:04 Koc

I've updated docs and fixed Psalm issues. Tests on my way. Ready to another round of review, removed draft/wip labels.

@Chartman123 @susnux mates, I would like to ask you keep user data for the 1st iteration and improve this part in upcoming PRs :pray::

• in our organization we have very cheap and small server with tiny storage. That's why we prefer limit storage size per user and prevent growing of app data without control
• we are urgently need this feature asap (yesterday :smile: ). Another circle of development can significantly delay merge.

in ideal word we should add possibility to configure how to store attachments (in app data or in user data) via config.php or via form settings. But this can be done via separate PR

PS: can anybody suggest me how to deal with this error?

Primary index name on "oc_forms_v2_uploaded_files" is too long.

I've explicitly provided index name primary but id didn't help.

and here empty log file attached to an artifacts of Cypress test, so I can't understand what an error.

Apr 30 '24 09:04 Koc

Codecov Report

Attention: Patch coverage is 59.28144% with 136 lines in your changes are missing coverage. Please review.

Project coverage is 45.95%. Comparing base (978ddc7) to head (6720298).

@@             Coverage Diff              @@
##               main    #2040      +/-   ##
============================================
+ Coverage     45.02%   45.95%   +0.92%     
- Complexity      720      797      +77     
============================================
  Files            61       65       +4     
  Lines          2776     3066     +290     
============================================
+ Hits           1250     1409     +159     
- Misses         1526     1657     +131     

May 01 '24 11:05 codecov[bot]

Awesome, we have completely green pipeline tonight :partying_face:

May 03 '24 21:05 Koc

@Koc I think the overall implementation is good, but there are a few points that I think are problemantic:

1. Validation: We validate all inputs before submitting, so also files should be validated (as good as possible) before submitting. This also makes it not necessary to translate API messages.
2. Why using UUIDs? We do not use them for any entity, if there is no good reason for introducing them I would simply go for numeric IDs.

But other than that is looks quite good :)

May 07 '24 12:05 susnux

@susnux @Chartman123 I did all changes

May 22 '24 23:05 Koc

Awesome work @Koc!

I deployed this PR locally and regarding usability and UI this is looking really great.

We also need this feature, would really appreciate to see this in the coming release.

May 24 '24 10:05 libnewton

I've added folder sharing with user when you're sharing form

May 24 '24 21:05 Koc

conflicts fixed

Jun 01 '24 11:06 Koc