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

Error when saving form with repeated upload inputs #8072

Open
dianabarsan opened this issue Feb 13, 2023 · 5 comments
Open

Error when saving form with repeated upload inputs #8072

dianabarsan opened this issue Feb 13, 2023 · 5 comments

Comments

@dianabarsan
Copy link
Member

Describe the bug
When a form with repeated uploads is submitted, an error is triggered and the report is not saved.

To Reproduce
Steps to reproduce the behavior:

  1. Launch 4.x
  2. Upload this form: https://gist.github.com/dianabarsan/3696d99ba17fc2ce462d58149618b93b
  3. Open the form and add more than one repeat. Save.
  4. See error

Expected behavior
The form should be saved.

Logs

feedback.service.ts:79 Error submitting form data:  TypeError: Cannot read properties of undefined (reading 'files')
    at Element.<anonymous> (enketo.service.ts:616:32)
    at Function.each (jquery.js:381:19)
    at jQuery.fn.init.each (jquery.js:203:17)
    at EnketoService.xmlToDocs (enketo.service.ts:613:8)
    at enketo.service.ts:796:21
    at _ZoneDelegate.invoke (zone.js:372:26)
    at Zone.run (zone.js:134:43)
    at zone.js:1275:36
    at _ZoneDelegate.invokeTask (zone.js:406:31)
    at Zone.runTask (zone.js:178:47)

Error triggered here:

const file = $input[0].files[0];

Screenshots
image

Environment

  • App: webapp
  • Version: 4.0+
@garethbowen
Copy link
Member

The problem is because the Xpath.getElementXPath(element); returns the path with the array position, eg: /upload_repeat/repeat[1]/my_photo but the name of the input does not, eg: /upload_repeat/repeat/my_photo. This means when we look up the file input it's not found.

There's not obvious way to get to the html input.

@garethbowen
Copy link
Member

@jkuester I'd appreciate your input on this potential fix when you have a moment. This approach is nice because it uses enketos own file manager to get the files. However it's not ideal because it changes the file format which means supporting multiple formats (legacy and new) forever. Also I don't think we can use the same approach for the binary type because it doesn't have a file name so file upload and binary will not be consistent.

Obviously this needs a lot of cleanup, testing, etc, but I wanted to run it past you first.

@garethbowen garethbowen self-assigned this Dec 31, 2023
@jkuester
Copy link
Contributor

jkuester commented Jan 4, 2024

So, I am definitely not intimate with this particular section of the code, but in general I think it is better to let the Enketo code handle complexity around repeat groups and not try to roll our own logic for that!

Changing the naming convention of the attachment is unfortunate, but if I understand it correctly we would have to do that anyway to support files within repeats (since our current naming convention does not account for multiple attachments for the "same" (repeated) question. Is that correct? Also, would we still be using the old naming convention for the binary attachments?

@garethbowen
Copy link
Member

if I understand it correctly we would have to do that anyway to support files within repeats (since our current naming convention does not account for multiple attachments for the "same" (repeated) question. Is that correct?

Not exactly. Because uploads don't work in repeats right now the old filename could still work and we could add square brackets and numbers when there are repeats, eg:

  • user-file/a/b (no repeat)
  • user-file/a[0]/b (repeat)

So allowing repeats would be an extension of the current filename system and be backwards compatible with the old non-repeat format.

Also, would we still be using the old naming convention for the binary attachments?

That was my thinking, but now that you bring it up I guess it means that repeats won't work for binaries even after this change. I'll have another look and see if I can find a deterministic name for the binary inputs.

@garethbowen
Copy link
Member

Somewhat related: #7530

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment