FEAT: add FileUpload and MultipleFileUpload fields in forms#44
Open
corentinbettiol wants to merge 3 commits intodjango-cms:mainfrom
Open
FEAT: add FileUpload and MultipleFileUpload fields in forms#44corentinbettiol wants to merge 3 commits intodjango-cms:mainfrom
corentinbettiol wants to merge 3 commits intodjango-cms:mainfrom
Conversation
Contributor
Reviewer's GuideImplements single and multiple file upload support in the form builder, including server- and client-side validation presets, storage/serialization of uploaded files into FormEntry, admin display/preservation of file data, AJAX handling fixes for multipart submissions, and new CMS plugins, forms, and tests around these features. Sequence diagram for AJAX file upload, validation, and storagesequenceDiagram
actor User
participant BrowserForm as Browser_form
participant AjaxJS as ajax_form_js
participant View as FormPluginView_ajax_post
participant Plugin as FileFieldPlugin
participant DjangoForm as Django_form
participant FileField as ValidatedFileField
participant MultiFileField as MultipleUploadedFilesField
participant Validator as validate_form_builder_file
participant Storage as default_storage
participant Entry as FormEntry
participant Serializer as serialize_cleaned_data_for_entry
User->>BrowserForm: Select file(s) and submit
BrowserForm->>AjaxJS: Submit event
AjaxJS->>AjaxJS: Build FormData(node)
AjaxJS->>View: fetch(action, method=POST, body=FormData, multipart)
View->>View: plugin_instance(instance_id)
View->>Plugin: ajax_post(request, params)
Plugin->>DjangoForm: instantiate(request=request)
DjangoForm->>FileField: clean(uploaded_file)
FileField->>Validator: validate_form_builder_file(file, preset_keys, user, request, field_name)
Validator-->>FileField: ok or FileValidationError
DjangoForm->>MultiFileField: clean(uploaded_files)
MultiFileField->>MultiFileField: iterate files
MultiFileField->>Validator: validate_form_builder_file(each_file, preset_keys, user, request, field_name)
Validator-->>MultiFileField: ok or FileValidationError
DjangoForm-->>Plugin: cleaned_data or errors
Note over Plugin,Entry: On success
Plugin->>Entry: FormEntry.objects.create(entry_data=serialize_cleaned_data_for_entry(cleaned_data))
Serializer->>Storage: default_storage.save(path, uploaded_file)
Storage-->>Serializer: url
Serializer-->>Entry: file metadata dict(s)
Note over Plugin,AjaxJS: On error
Plugin-->>View: JSON field_errors
View-->>AjaxJS: JSON response
AjaxJS->>BrowserForm: feedback(node, data)
AjaxJS->>BrowserForm: add .is-invalid, .invalid-feedback for fields
Class diagram for new file upload fields and validation presetsclassDiagram
class FormField {
+config
+field_name
+initialize_from_form(form)
+get_admin_fieldsets()
+get_admin_form()
+get_form_field(request)
}
class FileField {
+Meta
+get_form_field(request)
}
class MultipleFileField {
+Meta
+get_form_field(request)
}
FormField <|-- FileField
FormField <|-- MultipleFileField
class FileFieldForm {
+field_file_validation_presets
+__init__(*args, **kwargs)
}
class MultipleFileFieldForm {
+field_file_validation_presets
+__init__(*args, **kwargs)
}
class MultiFileInput {
+allow_multiple_selected
}
class ValidatedFileField {
-preset_keys: list
-_user
-_request
-_field_name: str
+__init__(preset_keys, user, request, field_name, **kwargs)
+clean(data, initial)
}
class MultipleUploadedFilesField {
+needs_multipart_form
-preset_keys: list
-_user
-_request
-_field_name: str
+__init__(preset_keys, user, request, field_name, **kwargs)
+clean(value)
}
ValidatedFileField --> MultiFileInput : widget.accept
MultipleUploadedFilesField --> MultiFileInput : widget
class BaseFilePresetValidator {
+__call__(uploaded_file, user, request, field_name)
+validate(uploaded_file, user, request, field_name)
}
class MaxSizePresetValidator {
-max_bytes: int
+__init__(max_bytes)
+validate(uploaded_file, user, request, field_name)
}
class ExtensionPresetValidator {
-allowed_extensions
+__init__(allowed_extensions)
+validate(uploaded_file, user, request, field_name)
}
class MimeFilenamePresetValidator {
-allowed_patterns
+__init__(allowed_patterns)
+validate(uploaded_file, user, request, field_name)
}
BaseFilePresetValidator <|-- MaxSizePresetValidator
BaseFilePresetValidator <|-- ExtensionPresetValidator
BaseFilePresetValidator <|-- MimeFilenamePresetValidator
class file_validation {
+FileValidationError
+get_validation_preset_registry()
+validation_preset_choice_tuples()
+allowed_extensions_for_accept_attribute(preset_keys)
+wrap_filer_style_validator(fn, validate_options)
+validate_form_builder_file(uploaded_file, preset_keys, user, request, field_name)
}
class file_validation_validators {
+enforce_max_size(uploaded_file, max_bytes, field_name)
+enforce_extension(uploaded_file, allowed_extensions, field_name)
+enforce_mime_from_filename(uploaded_file, allowed_patterns, field_name)
+_normalize_extension(ext)
+_format_size_limit(max_bytes)
}
ValidatedFileField --> file_validation : uses
MultipleUploadedFilesField --> file_validation : uses
file_validation ..> file_validation_validators : imports
file_validation ..> BaseFilePresetValidator : instantiates
class FormEntry {
+entry_data: JSONField
+get_file_entry_data_keys()
+get_admin_form()
+get_admin_fieldsets()
}
class FormEntryAdmin {
-_entry_file_key_by_attr: dict
+_entry_file_attr_name(key)
+_ensure_entry_file_attr_map(obj)
+get_readonly_fields(request, obj)
+get_fieldsets(request, obj)
+format_entry_file_field(obj, key)
+__getattr__(name)
+save_model(request, obj, form, change)
}
FormEntryAdmin --> FormEntry : manages
class form_entry_data {
+serialize_cleaned_data_for_entry(cleaned_data)
+_store_uploaded_file(uploaded_file)
}
form_entry_data --> FormEntry : populates_entry_data
class FileFieldPlugin {
+model: FileField
+form: FileFieldForm
+settings_fields
}
class MultipleFileFieldPlugin {
+model: MultipleFileField
+form: MultipleFileFieldForm
+settings_fields
}
FileFieldPlugin --> FileField
FileFieldPlugin --> FileFieldForm
MultipleFileFieldPlugin --> MultipleFileField
MultipleFileFieldPlugin --> MultipleFileFieldForm
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The logic that detects file-valued entry_data items is duplicated between
_is_file_entry_valueinentry_model.pyand the type checks inFormEntryAdmin.format_entry_file_field; consider centralizing this in a single helper to avoid divergence if the file metadata format changes. - In
allowed_extensions_for_accept_attribute, presets that don’t resolve toExtensionPresetValidator(or missing keys) are silently ignored, which may lead to mismatches between the client-sideacceptattribute and the actual server-side validation; consider making this asymmetry explicit (e.g., via a comment or defensive checks) or tightening the mapping so the HTML accept reflects all extension-based presets in use.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic that detects file-valued entry_data items is duplicated between `_is_file_entry_value` in `entry_model.py` and the type checks in `FormEntryAdmin.format_entry_file_field`; consider centralizing this in a single helper to avoid divergence if the file metadata format changes.
- In `allowed_extensions_for_accept_attribute`, presets that don’t resolve to `ExtensionPresetValidator` (or missing keys) are silently ignored, which may lead to mismatches between the client-side `accept` attribute and the actual server-side validation; consider making this asymmetry explicit (e.g., via a comment or defensive checks) or tightening the mapping so the HTML accept reflects all extension-based presets in use.
## Individual Comments
### Comment 1
<location path="djangocms_form_builder/file_validation.py" line_range="158-167" />
<code_context>
+ return wrapped
+
+
+def validate_form_builder_file(
+ uploaded_file: "UploadedFile",
+ preset_keys: list,
+ *,
+ user: "User",
+ request: typing.Optional["HttpRequest"],
+ field_name: str,
+) -> None:
+ """Run each preset callable in order for the given keys."""
+ if not preset_keys:
+ return
+ registry = get_validation_preset_registry()
+ for key in preset_keys:
+ entry = registry[key]
+ target = import_string(entry["validate"])
+ opts = entry.get("validate_options") or {}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Accessing `registry[key]` directly will raise a bare `KeyError` for unknown preset keys, which may be hard to diagnose.
In `validate_form_builder_file`, a missing key in `DJANGOCMS_FORM_BUILDER_FILE_VALIDATION_PRESETS` will currently bubble up as a bare `KeyError`, giving a 500 with little context if a preset is removed/renamed. Consider mapping this to `ImproperlyConfigured` with a clearer message, mirroring the existing `filer_validator` check, e.g.
```python
try:
entry = registry[key]
except KeyError as exc:
raise ImproperlyConfigured(
f"Unknown file validation preset key '{key}' in DJANGOCMS_FORM_BUILDER_FILE_VALIDATION_PRESETS"
) from exc
```
Suggested implementation:
```python
for key in preset_keys:
try:
entry = registry[key]
except KeyError as exc:
raise ImproperlyConfigured(
f"Unknown file validation preset key '{key}' in "
"DJANGOCMS_FORM_BUILDER_FILE_VALIDATION_PRESETS"
) from exc
target = import_string(entry["validate"])
opts = entry.get("validate_options") or {}
```
1. Ensure `ImproperlyConfigured` is imported at the top of `djangocms_form_builder/file_validation.py`, e.g.:
`from django.core.exceptions import ImproperlyConfigured`
2. If the file already imports `ImproperlyConfigured` elsewhere, no further changes are needed.
</issue_to_address>
### Comment 2
<location path="tests/test_upload_fields.py" line_range="112-121" />
<code_context>
+class MultipleUploadedFilesFieldTests(SimpleTestCase):
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for required/optional behavior of MultipleUploadedFilesField.clean.
Please add tests to cover the required/optional behavior of `MultipleUploadedFilesField.clean`, e.g.: (1) `required=False` with `value=None`/`[]` returns `[]`, and (2) `required=True` with `value=None`/`[]` raises `ValidationError` with the `"required"` message. This will guard against regressions in the custom `clean` implementation, which bypasses the default `FileField` logic.
Suggested implementation:
```python
field = ValidatedFileField(
preset_keys=["wide", "narrow"],
user=None,
request=request,
field_name="f",
required=False,
)
self.assertEqual(field.widget.attrs["accept"], ".pdf")
class MultipleUploadedFilesFieldRequiredBehaviorTests(SimpleTestCase):
def test_clean_optional_allows_empty(self):
field = MultipleUploadedFilesField(required=False)
self.assertEqual(field.clean(None), [])
self.assertEqual(field.clean([]), [])
def test_clean_required_rejects_empty(self):
field = MultipleUploadedFilesField(required=True)
with self.assertRaisesMessage(ValidationError, field.error_messages["required"]):
field.clean(None)
with self.assertRaisesMessage(ValidationError, field.error_messages["required"]):
field.clean([])
class MultipleUploadedFilesFieldTests(SimpleTestCase):
@override_settings(
DJANGOCMS_FORM_BUILDER_FILE_VALIDATION_PRESETS={
"ok": {"label": "OK", "validate": f"{__name__}.preset_accept"},
}
)
def test_clean_validates_each_file(self):
a = SimpleUploadedFile("a.txt", b"x", content_type="text/plain")
b = SimpleUploadedFile("b.txt", b"y", content_type="text/plain")
request = RequestFactory().get("/")
field = MultipleUploadedFilesField(
```
1. Ensure `ValidationError` is imported in `tests/test_upload_fields.py` (typically via `from django.core.exceptions import ValidationError`). If it's already imported for other tests, no change is needed.
2. If `MultipleUploadedFilesField` requires additional positional or keyword arguments beyond `required`, update the field construction in `MultipleUploadedFilesFieldRequiredBehaviorTests` to match the existing usage pattern (e.g., add `request`, `user`, or `preset_keys` as appropriate). The tests only rely on `clean()` being called with empty values, so other arguments can use minimal valid values.
</issue_to_address>
### Comment 3
<location path="tests/test_form_entry_data.py" line_range="11-16" />
<code_context>
+class SerializeCleanedDataTests(TestCase):
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test to ensure non-file lists are not treated as file uploads.
Current tests cover scalars and lists of `UploadedFile`s, but not lists of non-files (e.g. multi-select or checkbox values). Please add a case like `{"choices": ["a", "b"]}` that verifies the list is preserved and not treated as file upload data, to guard against future regressions in list handling.
```suggestion
class SerializeCleanedDataTests(TestCase):
def test_passes_through_non_files(self):
data = serialize_cleaned_data_for_entry({"a": "text", "b": 1})
self.assertEqual(data, {"a": "text", "b": 1})
def test_passes_through_non_file_lists(self):
cleaned_data = {"choices": ["a", "b"]}
data = serialize_cleaned_data_for_entry(cleaned_data)
self.assertEqual(data, cleaned_data)
def test_stores_upload_metadata(self):
```
</issue_to_address>
### Comment 4
<location path="djangocms_form_builder/admin.py" line_range="25" />
<code_context>
kwargs["form"] = obj.get_admin_form()
return super().get_form(request, obj, **kwargs)
+ @staticmethod
+ def _entry_file_attr_name(key):
+ safe = re.sub(r"[^a-zA-Z0-9_]", "_", str(key))
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing the file-entry detection and normalization logic into a shared helper so that `format_entry_file_field` only handles rendering of a normalized structure.
You can trim some of the complexity without changing behavior by centralizing the “file payload” detection/normalization logic instead of re‑implementing it in `format_entry_file_field`.
Right now `format_entry_file_field` repeats the structural checks that (per the other review) already exist in `entry_model.py` (`_is_file_entry_value` / similar). That makes the admin behavior depend on two copies of the same protocol, which increases the cognitive load and maintenance risk.
A small, focused refactor would be:
1. Move the structure inspection into a single shared helper on the model (or reuse the existing one if it already exists).
2. Have `format_entry_file_field` deal only with rendering, based on a normalized representation.
For example, on `FormEntry` (or in a shared utility), introduce a normalizer:
```python
# models.py
class FormEntry(models.Model):
...
@staticmethod
def get_file_entry_items(value):
"""
Normalize an entry_data value that may represent a single file
or a list of files into a list of file dicts, or [] if not a file.
"""
if isinstance(value, dict) and value.get("_form_builder_file"):
return [value]
if (
isinstance(value, list)
and value
and isinstance(value[0], dict)
and value[0].get("_form_builder_file")
):
return value
return []
```
Then in your admin:
```python
# admin.py
from .models import FormEntry
@staticmethod
def format_entry_file_field(obj, key):
items = FormEntry.get_file_entry_items(obj.entry_data.get(key))
if not items:
return "—"
if len(items) == 1:
d = items[0]
return format_html(
'<a href="{}" target="_blank" rel="noopener noreferrer">{}</a>',
d["url"],
d["filename"],
)
return format_html(
"<ul>{}</ul>",
format_html_join(
"",
'<li><a href="{}" target="_blank" rel="noopener noreferrer">{}</a></li>',
((d["url"], d["filename"]) for d in items),
),
)
```
If you already have `_is_file_entry_value` or similar, you can adapt `get_file_entry_items` to call that instead of duplicating its conditions. This keeps the admin-side logic focused on rendering and removes the duplicated protocol knowledge about the `_form_builder_file` structure.
</issue_to_address>
### Comment 5
<location path="djangocms_form_builder/file_validation_validators.py" line_range="131" />
<code_context>
+ )
+
+
+class BaseFilePresetValidator:
+ """
+ Subclass and implement :meth:`validate`, or use the concrete validators below.
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the preset validator classes to share a single data-driven base implementation that delegates to the existing helper functions instead of duplicating similar validate and __init__ logic in each subclass.
The main extra complexity is the three nearly-identical preset validator classes that only delegate to the helper functions. You can keep the OO preset API but reduce duplication by making the base class data‑driven and letting subclasses just configure which helper and option names to use.
For example:
```python
class BaseFilePresetValidator:
"""
Base preset: subclasses only define `helper` and `option_name`.
"""
helper: typing.Callable[..., None]
option_name: str
def __init__(self, **options):
# Enforce that the expected option is present
try:
self._value = options[self.option_name]
except KeyError:
raise TypeError(
f"{self.__class__.__name__} requires option {self.option_name!r}"
)
def __call__(self, uploaded_file, *, user, request, field_name: str) -> None:
self.validate(
uploaded_file,
user=user,
request=request,
field_name=field_name,
)
def validate(self, uploaded_file, *, user, request, field_name: str) -> None:
# Single shared implementation
self.helper(uploaded_file, self._value, field_name=field_name)
```
Concrete presets then become thin, declarative wrappers:
```python
class MaxSizePresetValidator(BaseFilePresetValidator):
helper = enforce_max_size
option_name = "max_bytes"
class ExtensionPresetValidator(BaseFilePresetValidator):
helper = enforce_extension
option_name = "allowed_extensions"
class MimeFilenamePresetValidator(BaseFilePresetValidator):
helper = enforce_mime_from_filename
option_name = "allowed_patterns"
```
This keeps:
- The same preset class names and callable interface used by the preset system.
- The functional helpers as the single place where validation logic lives.
But it removes three almost identical `validate()` implementations and the separate per‑class `__init__`, reducing the number of concepts the reader has to hold while keeping all current behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+112
to
+121
| class MultipleUploadedFilesFieldTests(SimpleTestCase): | ||
| @override_settings( | ||
| DJANGOCMS_FORM_BUILDER_FILE_VALIDATION_PRESETS={ | ||
| "ok": {"label": "OK", "validate": f"{__name__}.preset_accept"}, | ||
| } | ||
| ) | ||
| def test_clean_validates_each_file(self): | ||
| a = SimpleUploadedFile("a.txt", b"x", content_type="text/plain") | ||
| b = SimpleUploadedFile("b.txt", b"y", content_type="text/plain") | ||
| request = RequestFactory().get("/") |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add tests for required/optional behavior of MultipleUploadedFilesField.clean.
Please add tests to cover the required/optional behavior of MultipleUploadedFilesField.clean, e.g.: (1) required=False with value=None/[] returns [], and (2) required=True with value=None/[] raises ValidationError with the "required" message. This will guard against regressions in the custom clean implementation, which bypasses the default FileField logic.
Suggested implementation:
field = ValidatedFileField(
preset_keys=["wide", "narrow"],
user=None,
request=request,
field_name="f",
required=False,
)
self.assertEqual(field.widget.attrs["accept"], ".pdf")
class MultipleUploadedFilesFieldRequiredBehaviorTests(SimpleTestCase):
def test_clean_optional_allows_empty(self):
field = MultipleUploadedFilesField(required=False)
self.assertEqual(field.clean(None), [])
self.assertEqual(field.clean([]), [])
def test_clean_required_rejects_empty(self):
field = MultipleUploadedFilesField(required=True)
with self.assertRaisesMessage(ValidationError, field.error_messages["required"]):
field.clean(None)
with self.assertRaisesMessage(ValidationError, field.error_messages["required"]):
field.clean([])
class MultipleUploadedFilesFieldTests(SimpleTestCase):
@override_settings(
DJANGOCMS_FORM_BUILDER_FILE_VALIDATION_PRESETS={
"ok": {"label": "OK", "validate": f"{__name__}.preset_accept"},
}
)
def test_clean_validates_each_file(self):
a = SimpleUploadedFile("a.txt", b"x", content_type="text/plain")
b = SimpleUploadedFile("b.txt", b"y", content_type="text/plain")
request = RequestFactory().get("/")
field = MultipleUploadedFilesField(- Ensure
ValidationErroris imported intests/test_upload_fields.py(typically viafrom django.core.exceptions import ValidationError). If it's already imported for other tests, no change is needed. - If
MultipleUploadedFilesFieldrequires additional positional or keyword arguments beyondrequired, update the field construction inMultipleUploadedFilesFieldRequiredBehaviorTeststo match the existing usage pattern (e.g., addrequest,user, orpreset_keysas appropriate). The tests only rely onclean()being called with empty values, so other arguments can use minimal valid values.
Comment on lines
+11
to
+16
| class SerializeCleanedDataTests(TestCase): | ||
| def test_passes_through_non_files(self): | ||
| data = serialize_cleaned_data_for_entry({"a": "text", "b": 1}) | ||
| self.assertEqual(data, {"a": "text", "b": 1}) | ||
|
|
||
| def test_stores_upload_metadata(self): |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add a test to ensure non-file lists are not treated as file uploads.
Current tests cover scalars and lists of UploadedFiles, but not lists of non-files (e.g. multi-select or checkbox values). Please add a case like {"choices": ["a", "b"]} that verifies the list is preserved and not treated as file upload data, to guard against future regressions in list handling.
Suggested change
| class SerializeCleanedDataTests(TestCase): | |
| def test_passes_through_non_files(self): | |
| data = serialize_cleaned_data_for_entry({"a": "text", "b": 1}) | |
| self.assertEqual(data, {"a": "text", "b": 1}) | |
| def test_stores_upload_metadata(self): | |
| class SerializeCleanedDataTests(TestCase): | |
| def test_passes_through_non_files(self): | |
| data = serialize_cleaned_data_for_entry({"a": "text", "b": 1}) | |
| self.assertEqual(data, {"a": "text", "b": 1}) | |
| def test_passes_through_non_file_lists(self): | |
| cleaned_data = {"choices": ["a", "b"]} | |
| data = serialize_cleaned_data_for_entry(cleaned_data) | |
| self.assertEqual(data, cleaned_data) | |
| def test_stores_upload_metadata(self): |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #44 +/- ##
==========================================
- Coverage 81.88% 81.72% -0.17%
==========================================
Files 21 25 +4
Lines 1496 1844 +348
Branches 188 246 +58
==========================================
+ Hits 1225 1507 +282
- Misses 216 255 +39
- Partials 55 82 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix issue #9.
Add single file input field, and multiple files input field.
Also add validator code (and a few basic validators), and update doc to add how to create your own validator.
Add a setting named
DJANGOCMS_FORM_BUILDER_FILE_VALIDATION_PRESETS(a dict) which needs to contain all the validators + their config in order to let them be displayed on file fields options.Handle validators created for filer just fine (see updated docs for configuration options).
Screenshots
Summary by Sourcery
Add support for validated single and multiple file upload fields in the form builder, including storage, admin display, and AJAX handling updates.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: