[#1795] Add backend support for multiple file upload #855
Conversation
pi-sigma
force-pushed
the
feature/1795-doc-upload-backend
branch
2 times, most recently
from
2bc6822 to
6da0043
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #855 +/- ##
===========================================
- Coverage 92.93% 92.79% -0.15%
===========================================
Files 775 776 +1
Lines 26636 26713 +77
===========================================
+ Hits 24754 24787 +33
- Misses 1882 1926 +44 ☔ View full report in Codecov by Sentry. |
pi-sigma
force-pushed
the
feature/1795-doc-upload-backend
branch
from
6da0043 to
733a65b
Compare
| @@ -432,7 +432,7 @@ def test_cases(self, m): | |||
| upload_form = page.locator("#document-upload") | |||
| expect(upload_form).to_be_visible() | |||
|
|
|||
| file_input = upload_form.get_by_text(_("Sleep of selecteer bestand")) | |||
| file_input = page.locator("[name='file']") | |||
There was a problem hiding this comment.
I think we should prefer the get_by_label where possible (or any other selector that testes a11y).
There was a problem hiding this comment.
I changed the id and for attributes for the input and label elements to make the test work with get_by_label. Let me know if this is not desirable.
There was a problem hiding this comment.
I did do a small change, instead of using a hardcoded id I used the "auto_id" for both the input and the label.
| @@ -3,7 +3,7 @@ | |||
| <div class="form__control file-input"> | |||
| {% render_card direction="vertical" %} | |||
| {% icon icon="upload" icon_position="before" outlined=True %} | |||
| <input class="file-input__input" id="{{ field.auto_id }}" name="file" type="file"{% if field.field.required %} required{% endif %}> | |||
| <input class="file-input__input" id="{{ field.auto_id }}" name="file" type="file"{% if field.field.required %} required{% endif %} multiple> | |||
There was a problem hiding this comment.
Should this be dynamic based on the field type, and shouldn't we (dynamically) change the labels into plurals accordingly?
There was a problem hiding this comment.
multiple should be set dynamically and has been fixed. I'm not sure about the labels. There's no change in the backend if the client drops a file or two into the widget, so I'm not sure what could decide if the label should be singular or plural. Passing that info front to back seems overkill. I tend towards leaving it as is, or perhaps use an "optional plural" form like "Selecteer bestand(en)"
There was a problem hiding this comment.
I changed the label simply based on multiple. I don't think we need to have plural forms based on the length of the selection.
pi-sigma
force-pushed
the
feature/1795-doc-upload-backend
branch
5 times, most recently
from
02d41ae to
4678342
Compare
pi-sigma
force-pushed
the
feature/1795-doc-upload-backend
branch
from
4678342 to
998b97e
Compare
svenvandescheur
force-pushed
the
feature/1795-doc-upload-backend
branch
from
d0bf625 to
d11eb91
Compare
| created_relationship = connect_case_with_document( | ||
| self.case.url, created_document.get("url") | ||
| ) | ||
| except AttributeError: |
There was a problem hiding this comment.
@pi-sigma I've added the AttributeError handling here because created_document could be None, however I think we might want to separate the error handling for the both objects and only create the relationship if the document is actually created.
This might require some small refactoring here.
There was a problem hiding this comment.
@svenvandescheur I don't think we need to expose this to the user; they only care about whether the upload worked or not.
Relatedly, I tested the code again and it now fails with getattr for me too, as it should. So I think we're good to go.
pi-sigma
force-pushed
the
feature/1795-doc-upload-backend
branch
from
d11eb91 to
e35db34
Compare
Taiga: #1795
Front-end: PR #833