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

Add support for file question #2040

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

Koc
Copy link
Collaborator

@Koc Koc commented Apr 1, 2024

This PR introduces possibility add new File Question type:

Preview

image

image

image

image

image

image

image

image

image

image

image

image

image

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

@Chartman123 Chartman123 marked this pull request as draft April 2, 2024 09:00
@susnux
Copy link
Collaborator

susnux commented Apr 2, 2024

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...

@Chartman123 Chartman123 linked an issue Apr 2, 2024 that may be closed by this pull request
@susnux
Copy link
Collaborator

susnux commented Apr 2, 2024

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.

@Chartman123
Copy link
Collaborator

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:

grafik

Available file types:

grafik

The settings could be stored into our extraSettings

@Koc
Copy link
Collaborator Author

Koc commented Apr 2, 2024

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?

@Koc
Copy link
Collaborator Author

Koc commented Apr 2, 2024

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

@Chartman123
Copy link
Collaborator

Any ideas how to workaround that?

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

@Koc Koc force-pushed the feature/add-file-type-question branch from 2410888 to 3bffb99 Compare April 5, 2024 23:11
@Koc

This comment was marked as outdated.

lib/Constants.php Outdated Show resolved Hide resolved
lib/Constants.php Outdated Show resolved Hide resolved
@Chartman123
Copy link
Collaborator

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...

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I would say go with a modal here since it's quite a bunch of different settings. The small dropdown is too cramped of a space for that.

And agree on that we should go with the actual file names, and also "Forms/Form name", without UUID. Since this will be visible in the file system there should be no IDs visible – if the name exists, we can append " (2)" and count up like we do in Files and elsewhere too.

@Koc and also, awesome feature! That will be very useful. :)

@susnux
Copy link
Collaborator

susnux commented Apr 8, 2024

  • 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

@susnux
Copy link
Collaborator

susnux commented Apr 8, 2024

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.

@Chartman123
Copy link
Collaborator

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

@Koc Koc force-pushed the feature/add-file-type-question branch 3 times, most recently from ffbcf78 to d45d84c Compare April 12, 2024 09:12
@Koc
Copy link
Collaborator Author

Koc commented Apr 12, 2024

@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?

@Chartman123
Copy link
Collaborator

@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

@susnux
Copy link
Collaborator

susnux commented Apr 12, 2024

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.

@susnux
Copy link
Collaborator

susnux commented Apr 12, 2024

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).

@Chartman123
Copy link
Collaborator

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

@Koc Koc force-pushed the feature/add-file-type-question branch 3 times, most recently from 2650d5d to f862a87 Compare April 14, 2024 22:54
@Koc Koc force-pushed the feature/add-file-type-question branch 3 times, most recently from 2b84687 to 6c105a8 Compare May 27, 2024 10:49
@Koc Koc force-pushed the feature/add-file-type-question branch 2 times, most recently from f54bf2b to c0ce60e Compare June 1, 2024 11:53
@Koc
Copy link
Collaborator Author

Koc commented Jun 1, 2024

conflicts fixed

lib/Service/FormsService.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good to me right now.
Yet I would like to give this a longer test (also security) before releasing in the stable channel :)

@susnux susnux added enhancement New feature or request 3. to review Waiting for reviews labels Jun 1, 2024
@Koc Koc force-pushed the feature/add-file-type-question branch from c0ce60e to 3360cd9 Compare June 2, 2024 09:13
Copy link
Collaborator

@Chartman123 Chartman123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already tested some things like what happens, if the folder is already created manually by the user or shared by someone else. So far no problems (other than of course 'file not found' problems if you delete the folder (or subfolders).

So, you could just resolve the comment from my review and then also ready to go from my side :)

@susnux what do you mean with "longer testing"? Already merge it but don't release a new 4.3 version or still wait with merging?

lib/Constants.php Outdated Show resolved Hide resolved
@Chartman123
Copy link
Collaborator

I get the following warning in my Nextcloud log:

{
  "reqId": "jWDW9tqYEAOi5IViSsUD",
  "level": 2,
  "time": "2024-06-02T20:27:32+00:00",
  "remoteAddr": "192.168.1.132",
  "user": "admin",
  "app": "forms",
  "method": "POST",
  "url": "/nextcloud/ocs/v2.php/apps/forms/api/v2.4/submission/insert",
  "message": "Controller OCA\\Forms\\Controller\\ApiController::insertSubmission executed 251 queries.",
  "userAgent": "Mozilla/5.0 (X11; Linux x86_64; rv:126.0) Gecko/20100101 Firefox/126.0",
  "version": "29.0.2.1",
  "data": {
    "app": "forms"
  },
  "id": "665ce985a209b"
}

@Koc
Copy link
Collaborator Author

Koc commented Jun 3, 2024

@Chartman123 do you mean that we need somehow reduce amount of queries for sumbission insertion? How many queries do we have for another questions type?

Signed-off-by: Konstantin Myakshin <[email protected]>
@Koc Koc force-pushed the feature/add-file-type-question branch from 3360cd9 to 6720298 Compare June 3, 2024 14:11
@Chartman123
Copy link
Collaborator

@Koc I don't know... It's the first time I saw such a warning. Perhaps @susnux can say more about it and where this comes from

@Koc
Copy link
Collaborator Author

Koc commented Jun 3, 2024

Any idea why Cypress is failing after rebase?

Error: Action failed. Missing package manager lockfile. Expecting one of package-lock.json (npm), pnpm-lock.yaml (pnpm) or yarn.lock (yarn) in working-directory /home/runner/work/forms/forms

At the same time node lint works fine

@Koc
Copy link
Collaborator Author

Koc commented Jun 4, 2024

@susnux @Chartman123 so, there are 2 approvals, green pipeline. What else left to be merged?

@susnux
Copy link
Collaborator

susnux commented Jun 8, 2024

As @Koc said we should look for issues and fix them in next iteration.

Great work @Koc nice to have feature! 🚀

@susnux susnux merged commit 264b932 into nextcloud:main Jun 8, 2024
50 checks passed
@szaimen
Copy link

szaimen commented Jun 8, 2024

🎉🎉🎉🎉🎉

@Koc Koc deleted the feature/add-file-type-question branch June 8, 2024 13:02
@cloudkuba
Copy link

cloudkuba commented Jun 10, 2024

@Koc
Hi,
Does this change only allow files to be uploaded once the survey is open, or can I just add the file to the survey so that users can view it?

I need this functionality - the user must download the file to be able to send the survey.

@Chartman123
Copy link
Collaborator

Chartman123 commented Jun 10, 2024

@cloudkuba This PR is just about submitting files... Attaching files to the description of the form or a question can already be done with a markdown link: [Text](https://www.domain.tld/path/to/file)

@cloudkuba
Copy link

@cloudkuba This PR is just about submitting files... Attaching files to the description of the form or a question can already be done with a markdown link: [Text](https://www.domain.tld/path/to/file)

Hi, What I mean is more about verification that the file MUST be downloaded in order to proceed.

@Chartman123
Copy link
Collaborator

@cloudkuba no, this is not in our scope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Upload file" as question/response
7 participants