Simple fix for the CfP proposal progress issue #1551
+282
−10
Conversation
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
Contributor
Reviewer's GuideRefines the CFP multi-step form handling to preserve session-backed data and uploaded files across navigation, adds UI feedback for preserved uploads, handles the Back action via POST, hardens the proposal title JS against missing fields, and introduces client-side auto-save of form data using sessionStorage. Sequence diagram for the CFP Back action POST handlingsequenceDiagram
actor User
participant Browser
participant CfpSubmissionForm
participant CfpStepView
participant WizardView
participant SessionStore
User->>Browser: Click Back button
Browser->>CfpSubmissionForm: Submit form with action=back (POST)
CfpSubmissionForm->>WizardView: dispatch(request, POST, action=back)
WizardView->>CfpStepView: call step.post(request)
activate CfpStepView
CfpStepView->>CfpStepView: get_form()
CfpStepView->>SessionStore: get_form_initial(), get_files()
SessionStore-->>CfpStepView: initial data, session files
CfpStepView->>CfpStepView: merge session_files with request.FILES
CfpStepView->>CfpStepView: form.is_valid()
alt form is valid
CfpStepView->>SessionStore: set_data(form.cleaned_data)
end
alt form has files
CfpStepView->>SessionStore: set_files(form.files)
end
CfpStepView->>CfpStepView: get_prev_url(request)
CfpStepView-->>WizardView: redirect(prev_url or request.path)
deactivate CfpStepView
WizardView-->>Browser: redirect response to previous step
Browser-->>User: Load previous step page with preserved data/files
Sequence diagram for CFP form auto-save with sessionStoragesequenceDiagram
actor User
participant Browser
participant formAutoSaveJS
participant sessionStorage
participant Server
User->>Browser: Open CFP step page
Browser->>formAutoSaveJS: Load and execute script
formAutoSaveJS->>Browser: init() on DOM ready
formAutoSaveJS->>sessionStorage: getItem(cfp_form_data_path)
alt saved data exists and differs from server data
formAutoSaveJS->>Browser: restore form fields from saved data
else saved data matches server data or not present
formAutoSaveJS->>sessionStorage: removeItem(cfp_form_data_path)
end
User->>Browser: Type in form fields
Browser->>formAutoSaveJS: input/change events
formAutoSaveJS->>formAutoSaveJS: debounce(saveFormData, 500ms)
formAutoSaveJS->>sessionStorage: setItem(cfp_form_data_path, serialized formData)
User->>Browser: Navigate away (back/refresh/close)
Browser->>formAutoSaveJS: beforeunload event
formAutoSaveJS->>sessionStorage: setItem(cfp_form_data_path, latest formData)
User->>Browser: Return to same CFP step URL
Browser->>Server: GET CFP step
Server-->>Browser: HTML with current session-backed data
Browser->>formAutoSaveJS: init() on DOM ready
formAutoSaveJS->>sessionStorage: getItem(cfp_form_data_path)
alt saved data differs from current form values
formAutoSaveJS->>Browser: overwrite form fields with saved values
else matches
formAutoSaveJS->>sessionStorage: removeItem(cfp_form_data_path)
end
Updated class diagram for CFP step flow and wizard dispatchclassDiagram
class CfpStepView {
-request
-cfp_session
-form_class
-identifier
+get_form_initial() dict
+get_files() dict
+set_files(files) None
+set_data(cleaned_data) None
+get_prev_url(request) str
+get_form(from_storage bool) form
+get_context_data(**kwargs) dict
+post(request) HttpResponse
}
class WizardView {
+dispatch(request, *args, **kwargs) HttpResponse
+get_current_step(request) CfpStepView
+get_steps(request) list
}
CfpStepView ..> SessionStore : uses
WizardView --> CfpStepView : coordinates
class SessionStore {
+get(key) object
+set(key, value) None
}
File-Level Changes
Assessment against linked issues
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 there - I've reviewed your changes - here's some feedback:
- When merging
session_fileswithself.request.FILESfor POST, convertingFILESto a plain dict withdict(self.request.FILES.items())risks losing multi-value file fields (e.g.,MultiValueDictsemantics); consider preserving the original structure or explicitly handling multiple files per field if that’s a use case. - The logic in
get_formnow callsget_form_initial()multiple times per request path; consider storing the result in a local variable to avoid repeated work and keep the control flow easier to read.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When merging `session_files` with `self.request.FILES` for POST, converting `FILES` to a plain dict with `dict(self.request.FILES.items())` risks losing multi-value file fields (e.g., `MultiValueDict` semantics); consider preserving the original structure or explicitly handling multiple files per field if that’s a use case.
- The logic in `get_form` now calls `get_form_initial()` multiple times per request path; consider storing the result in a local variable to avoid repeated work and keep the control flow easier to read.
## Individual Comments
### Comment 1
<location> `app/eventyay/cfp/flow.py:239-245` </location>
<code_context>
**self.get_form_kwargs(),
)
- return self.form_class(data=self.request.POST, files=self.request.FILES, **self.get_form_kwargs())
+ # For POST requests, merge new uploads with existing session files
+ # This allows users to navigate back without losing previously uploaded files
+ session_files = self.get_files() or {}
+ new_files = dict(self.request.FILES.items())
+ # New uploads take precedence over session files
+ files = {**session_files, **new_files}
+ return self.form_class(data=self.request.POST, files=files, **self.get_form_kwargs())
def is_completed(self, request):
</code_context>
<issue_to_address>
**issue (bug_risk):** Merging file data into a plain dict may break MultiValueDict behavior for file fields.
`request.FILES` is a `MultiValueDict` and must support `getlist` for multi-file fields. Converting to a plain `dict` (`dict(self.request.FILES.items())` and `{**session_files, **new_files}`) removes that behavior and can break forms expecting `getlist` or multiple files per field. Please keep `files` as a `MultiValueDict` (e.g., construct a `MultiValueDict` and use `.update()` / `.setlistdefault()` when merging) so the interface is preserved.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The merge logic for existing and new uploaded files uses
session_files.items(), which on aMultiValueDictonly preserves a single value per key and can break multi-file fields; consider iterating via.lists()or copying theMultiValueDictdirectly to retain all files. - The
uploaded_filescontext currently maps a single filename per field, which may be misleading for fields that support multiple files; if multi-file uploads are allowed, consider exposing all associated filenames rather than only one.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The merge logic for existing and new uploaded files uses `session_files.items()`, which on a `MultiValueDict` only preserves a single value per key and can break multi-file fields; consider iterating via `.lists()` or copying the `MultiValueDict` directly to retain all files.
- The `uploaded_files` context currently maps a single filename per field, which may be misleading for fields that support multiple files; if multi-file uploads are allowed, consider exposing all associated filenames rather than only one.
## Individual Comments
### Comment 1
<location> `app/eventyay/cfp/flow.py:247-250` </location>
<code_context>
+ # This allows users to navigate back without losing previously uploaded files
+ session_files = self.get_files() or {}
+
+ # Preserve MultiValueDict semantics for proper multi-file field support
+ files = MultiValueDict()
+ # Add session files first
+ for field, file_obj in session_files.items():
+ files[field] = file_obj
+ # New uploads take precedence over session files
</code_context>
<issue_to_address>
**issue (bug_risk):** Merging session files may break multi-file fields when `get_files()` returns a `MultiValueDict`.
When `self.get_files()` is a `MultiValueDict`, using `.items()` and `files[field] = file_obj` collapses multiple values into one and skips `setlist`, so multi-file uploads can lose files or change the expected type. To preserve full multi-file support, iterate with `session_files.lists()` (or equivalent) and use `files.setlist(field, file_list)`, consistent with how you handle `request.FILES`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where previously uploaded files were lost when navigating back and forth between Call for Proposals (CfP) submission steps. The solution changes the "back" button from a link to a submit button, implements file merging logic to preserve session files, and adds a UI display showing previously uploaded files to users.
- Implements file preservation across multi-step CfP wizard navigation
- Changes back button to POST action to save data before navigating
- Merges session files with new uploads to prevent data loss
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
app/eventyay/cfp/views/wizard.py |
Adds special handling for "back" action to return early after data is saved |
app/eventyay/cfp/templates/cfp/event/submission_base.html |
Converts back button from link to submit button and adds display of previously uploaded files |
app/eventyay/cfp/flow.py |
Refactors form creation logic to distinguish GET/validation/POST cases, implements file merging to preserve session files, and handles back button with partial data saving |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/eventyay/cfp/flow.py
Outdated
Comment on lines
284
to
289
| # Save raw POST data for invalid forms (user might be going back to fix something) | ||
| # Filter out non-field data and serialize appropriately | ||
| partial_data = { | ||
| field_name: request.POST.get(field_name) | ||
| for field_name in form.fields.keys() | ||
| if field_name in request.POST |
There was a problem hiding this comment.
The comment mentions filtering out non-field data, but the implementation only filters based on whether field_name is in request.POST. This doesn't actually filter out non-field POST data like 'csrfmiddlewaretoken' or 'action'. Consider adding a check to exclude these special fields, or the comment should be updated to accurately reflect what the code does.
Comment on lines
243
to
257
| # For POST requests, merge new uploads with existing session files | ||
| # This allows users to navigate back without losing previously uploaded files | ||
| session_files = self.get_files() or {} | ||
|
|
||
| # Preserve MultiValueDict semantics for proper multi-file field support | ||
| files = MultiValueDict() | ||
| # Add session files first | ||
| for field, file_obj in session_files.items(): | ||
| files[field] = file_obj | ||
| # New uploads take precedence over session files | ||
| for field, file_list in self.request.FILES.lists(): | ||
| files.setlist(field, file_list) | ||
|
|
||
| return self.form_class(data=self.request.POST, files=files, **self.get_form_kwargs()) | ||
|
|
There was a problem hiding this comment.
There's a potential issue with file preservation logic. When a user clicks back without uploading any new files, the session files are merged into form.files (lines 243-256). However, these session files are UploadedFile objects pointing to already-saved temporary files. When set_files is called with these files on line 295, it will call file_storage.save again, creating duplicate temporary files in storage without cleaning up the originals. Consider checking if files come from session (e.g., by comparing with self.get_files()) before saving them again, or only save files that are actually new uploads from request.FILES.
Suggested change
| # For POST requests, merge new uploads with existing session files | |
| # This allows users to navigate back without losing previously uploaded files | |
| session_files = self.get_files() or {} | |
| # Preserve MultiValueDict semantics for proper multi-file field support | |
| files = MultiValueDict() | |
| # Add session files first | |
| for field, file_obj in session_files.items(): | |
| files[field] = file_obj | |
| # New uploads take precedence over session files | |
| for field, file_list in self.request.FILES.lists(): | |
| files.setlist(field, file_list) | |
| return self.form_class(data=self.request.POST, files=files, **self.get_form_kwargs()) | |
| # For POST requests, only use newly uploaded files from the request. | |
| # Previously uploaded files are preserved in session storage and | |
| # should not be passed back as fresh uploads to avoid re-saving | |
| # already stored temporary files. | |
| files = self.request.FILES or None | |
| return self.form_class( | |
| data=self.request.POST, | |
| files=files, | |
| **self.get_form_kwargs(), | |
| ) |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
hongquan
reviewed
Dec 19, 2025
Member
hongquan
left a comment
hongquan
left a comment
There was a problem hiding this comment.
By implementing the "Back" button as submit button, it refuses to navigate if I don't input any values to the form of 2nd step.
I don't know if this is acceptable for #1391
Other thing is that, the solution provided by this PR doesn't work with the browser's "Back" button, or Android "Back" button. I'm among the users who prefer to click the "Back" button of the browser or Android, because it is always visible.
- add listener to those events - save form data on back event - restore on reenter, depending on session data present - also fix an error in proposalTabTitles JS code
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- When merging existing session files in
get_formyou assignfiles[field] = file_objfor eachsession_files.items(), which will drop multiple files per field; consider iteratingsession_files.lists()(or checking for list-like values) and usingfiles.setlist(field, file_list)so multi-file uploads are preserved correctly. - The
formAutoSave.js"allFieldsMatch" heuristic inrestoreFormDatacompares only the first control's.valuefor each name and does not mirror the serialization logic insaveFormData, which can mis-detect whether the server echoed the submitted data; consider reusing the same serialization approach for both save and comparison to avoid false positives/negatives.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When merging existing session files in `get_form` you assign `files[field] = file_obj` for each `session_files.items()`, which will drop multiple files per field; consider iterating `session_files.lists()` (or checking for list-like values) and using `files.setlist(field, file_list)` so multi-file uploads are preserved correctly.
- The `formAutoSave.js` "allFieldsMatch" heuristic in `restoreFormData` compares only the first control's `.value` for each name and does not mirror the serialization logic in `saveFormData`, which can mis-detect whether the server echoed the submitted data; consider reusing the same serialization approach for both save and comparison to avoid false positives/negatives.
## Individual Comments
### Comment 1
<location> `app/eventyay/cfp/flow.py:247-253` </location>
<code_context>
+ # This allows users to navigate back without losing previously uploaded files
+ session_files = self.get_files() or {}
+
+ # Preserve MultiValueDict semantics for proper multi-file field support
+ files = MultiValueDict()
+ # Add session files first
+ for field, file_obj in session_files.items():
+ files[field] = file_obj
+ # For each field, new uploads completely replace any existing session files
+ for field, file_list in self.request.FILES.lists():
+ files.setlist(field, file_list)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Merging `session_files` into `MultiValueDict` can break multi-file fields and value types.
The merge assumes `session_files.items()` returns a single file per field, but `get_files()` likely returns a `MultiValueDict`-like object or lists for multi-file fields. Assigning with `files[field] = file_obj` causes `MultiValueDict.__setitem__` to wrap values in a list; if `file_obj` is already a list, you get nested lists. And if `session_files` is a `MultiValueDict`, iterating over `.items()` drops all but the last file per field.
To preserve multi-file semantics, build `files` using `setlist`/`lists()` instead of plain item assignment, e.g.:
```python
files = MultiValueDict()
if isinstance(session_files, MultiValueDict):
for field, file_list in session_files.lists():
files.setlist(field, list(file_list))
else:
for field, file_obj in session_files.items():
files[field] = file_obj
for field, file_list in self.request.FILES.lists():
files.setlist(field, file_list)
```
This preserves existing multi-file values and avoids nested lists when there’s no new upload for a field.
</issue_to_address>
### Comment 2
<location> `app/eventyay/static/cfp/js/formAutoSave.js:48-55` </location>
<code_context>
+ }
+ });
+
+ // Process all inputs
+ inputs.forEach((input) => {
+ const name = input.name;
+ const type = input.type;
+
+ if (!name || name === 'csrfmiddlewaretoken' || name === 'action') return;
+
+ if (type === 'checkbox') {
+ formData[name] = input.checked;
+ } else if (type === 'radio') {
</code_context>
<issue_to_address>
**issue (bug_risk):** Checkbox groups with the same name are not handled correctly when saving/restoring.
The current logic stores a single boolean per checkbox name (`formData[name] = input.checked`), so in a group only the last processed checkbox controls the stored value. On restore, that single boolean is then applied to every checkbox in the group, losing the actual selection.
For checkbox groups that represent multi-select fields, store an array of checked values keyed by `name`, similar to `multiple` selects, and restore based on `value`:
```js
// saveFormData
if (type === 'checkbox') {
if (!formData[name]) formData[name] = [];
if (input.checked) formData[name].push(input.value);
}
```
```js
// restore
if (elements.length > 1 && elements[0].type === 'checkbox' && Array.isArray(value)) {
for (let i = 0; i < elements.length; i++) {
elements[i].checked = value.includes(elements[i].value);
}
continue;
}
```
This preserves the correct set of checked options per group.
</issue_to_address>
### Comment 3
<location> `app/eventyay/static/cfp/js/formAutoSave.js:98-107` </location>
<code_context>
+ // Check if the saved sessionStorage data matches the current form data
+ // If they match exactly, it means we successfully submitted and the server
+ // echoed back our data - in this case, clear sessionStorage
+ let allFieldsMatch = true;
+ let checkedFields = 0;
+
+ for (const [name, savedValue] of Object.entries(formData)) {
+ const elements = form.elements[name];
+ if (!elements) continue;
+
+ const element = elements.length !== undefined ? elements[0] : elements;
+ const currentValue = element.value || '';
+ const savedValueStr = String(savedValue || '');
+
+ checkedFields++;
+ if (currentValue.trim() !== savedValueStr.trim()) {
+ allFieldsMatch = false;
+ break;
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The "all fields match" detection for clearing sessionStorage is likely too weak and may never trigger.
This comparison only uses `element.value` from the first control per `name`, so it ignores checked state for checkboxes/radios and multi-select values, and may treat matching submissions as mismatches. That means `allFieldsMatch` can stay `false` even when the server echoed the submitted data, leaving stale sessionStorage around.
To make this reliable, derive `currentData` the same way as in `saveFormData()` and compare the serialized forms (e.g. `JSON.stringify(currentData) === savedData`) instead of relying on `form.elements[name][0].value`.
Suggested implementation:
```javascript
// Check if the saved sessionStorage data matches the current form data.
// If they match exactly, it means we successfully submitted and the server
// echoed back our data - in this case, clear sessionStorage.
// We reconstruct the current form data in the same shape as what we store.
const currentData = {};
const formDataFromDom = new FormData(form);
for (const [name, value] of formDataFromDom.entries()) {
if (Object.prototype.hasOwnProperty.call(currentData, name)) {
const existing = currentData[name];
if (Array.isArray(existing)) {
existing.push(value);
} else {
currentData[name] = [existing, value];
}
} else {
currentData[name] = value;
}
}
const serializedCurrentData = JSON.stringify(currentData);
// If the serialized current form data matches what we have stored,
// it means the form was successfully submitted - clear sessionStorage.
if (serializedCurrentData === savedData) {
clearFormData();
return;
}
```
To be fully robust and future-proof, you should ensure that `saveFormData()` uses the same normalization/serialization logic as this block. The ideal approach is:
1. Extract the `currentData` building logic into a shared helper (e.g. `getNormalizedFormData(form)`).
2. Use that helper both in `saveFormData()` (before `sessionStorage.setItem`) and here (before comparison), so any changes to how form data is represented are made in one place.
</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
+247
to
+253
| # Preserve MultiValueDict semantics for proper multi-file field support | ||
| files = MultiValueDict() | ||
| # Add session files first | ||
| for field, file_obj in session_files.items(): | ||
| files[field] = file_obj | ||
| # For each field, new uploads completely replace any existing session files | ||
| for field, file_list in self.request.FILES.lists(): |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Merging session_files into MultiValueDict can break multi-file fields and value types.
The merge assumes session_files.items() returns a single file per field, but get_files() likely returns a MultiValueDict-like object or lists for multi-file fields. Assigning with files[field] = file_obj causes MultiValueDict.__setitem__ to wrap values in a list; if file_obj is already a list, you get nested lists. And if session_files is a MultiValueDict, iterating over .items() drops all but the last file per field.
To preserve multi-file semantics, build files using setlist/lists() instead of plain item assignment, e.g.:
files = MultiValueDict()
if isinstance(session_files, MultiValueDict):
for field, file_list in session_files.lists():
files.setlist(field, list(file_list))
else:
for field, file_obj in session_files.items():
files[field] = file_obj
for field, file_list in self.request.FILES.lists():
files.setlist(field, file_list)This preserves existing multi-file values and avoids nested lists when there’s no new upload for a field.
Comment on lines
+48
to
+55
| // Process all inputs | ||
| inputs.forEach((input) => { | ||
| const name = input.name; | ||
| const type = input.type; | ||
|
|
||
| if (!name || name === 'csrfmiddlewaretoken' || name === 'action') return; | ||
|
|
||
| if (type === 'checkbox') { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Checkbox groups with the same name are not handled correctly when saving/restoring.
The current logic stores a single boolean per checkbox name (formData[name] = input.checked), so in a group only the last processed checkbox controls the stored value. On restore, that single boolean is then applied to every checkbox in the group, losing the actual selection.
For checkbox groups that represent multi-select fields, store an array of checked values keyed by name, similar to multiple selects, and restore based on value:
// saveFormData
if (type === 'checkbox') {
if (!formData[name]) formData[name] = [];
if (input.checked) formData[name].push(input.value);
}// restore
if (elements.length > 1 && elements[0].type === 'checkbox' && Array.isArray(value)) {
for (let i = 0; i < elements.length; i++) {
elements[i].checked = value.includes(elements[i].value);
}
continue;
}This preserves the correct set of checked options per group.
Comment on lines
+98
to
+107
| let allFieldsMatch = true; | ||
| let checkedFields = 0; | ||
|
|
||
| for (const [name, savedValue] of Object.entries(formData)) { | ||
| const elements = form.elements[name]; | ||
| if (!elements) continue; | ||
|
|
||
| const element = elements.length !== undefined ? elements[0] : elements; | ||
| const currentValue = element.value || ''; | ||
| const savedValueStr = String(savedValue || ''); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): The "all fields match" detection for clearing sessionStorage is likely too weak and may never trigger.
This comparison only uses element.value from the first control per name, so it ignores checked state for checkboxes/radios and multi-select values, and may treat matching submissions as mismatches. That means allFieldsMatch can stay false even when the server echoed the submitted data, leaving stale sessionStorage around.
To make this reliable, derive currentData the same way as in saveFormData() and compare the serialized forms (e.g. JSON.stringify(currentData) === savedData) instead of relying on form.elements[name][0].value.
Suggested implementation:
// Check if the saved sessionStorage data matches the current form data.
// If they match exactly, it means we successfully submitted and the server
// echoed back our data - in this case, clear sessionStorage.
// We reconstruct the current form data in the same shape as what we store.
const currentData = {};
const formDataFromDom = new FormData(form);
for (const [name, value] of formDataFromDom.entries()) {
if (Object.prototype.hasOwnProperty.call(currentData, name)) {
const existing = currentData[name];
if (Array.isArray(existing)) {
existing.push(value);
} else {
currentData[name] = [existing, value];
}
} else {
currentData[name] = value;
}
}
const serializedCurrentData = JSON.stringify(currentData);
// If the serialized current form data matches what we have stored,
// it means the form was successfully submitted - clear sessionStorage.
if (serializedCurrentData === savedData) {
clearFormData();
return;
}To be fully robust and future-proof, you should ensure that saveFormData() uses the same normalization/serialization logic as this block. The ideal approach is:
- Extract the
currentDatabuilding logic into a shared helper (e.g.getNormalizedFormData(form)). - Use that helper both in
saveFormData()(beforesessionStorage.setItem) and here (before comparison), so any changes to how form data is represented are made in one place.
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
formAutoSave.jslogic treats checkboxes and radios as single values when saving but as potentially multiple elements when restoring, which will break fields with multiple checkboxes sharing the same name; consider storing checkbox values as arrays keyed by value instead of a single boolean. - The
restoreFormDataheuristic for detecting a successful submission only compares the first element for grouped fields (e.g., radios/checkboxes), which can lead to incorrectly clearingsessionStorage; it would be safer to compare the full group or limit this check to simple scalar fields.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `formAutoSave.js` logic treats checkboxes and radios as single values when saving but as potentially multiple elements when restoring, which will break fields with multiple checkboxes sharing the same name; consider storing checkbox values as arrays keyed by value instead of a single boolean.
- The `restoreFormData` heuristic for detecting a successful submission only compares the first element for grouped fields (e.g., radios/checkboxes), which can lead to incorrectly clearing `sessionStorage`; it would be safer to compare the full group or limit this check to simple scalar fields.
## Individual Comments
### Comment 1
<location> `app/eventyay/cfp/flow.py:245-250` </location>
<code_context>
- return self.form_class(data=self.request.POST, files=self.request.FILES, **self.get_form_kwargs())
+ # For POST requests, merge new uploads with existing session files
+ # This allows users to navigate back without losing previously uploaded files
+ session_files = self.get_files() or {}
+
+ # Preserve MultiValueDict semantics for proper multi-file field support
+ files = MultiValueDict()
+ # Add session files first
+ for field, file_obj in session_files.items():
+ files[field] = file_obj
+ # For each field, new uploads completely replace any existing session files
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential loss of multi-file values when restoring session files into MultiValueDict
If `self.get_files()` can return a `MultiValueDict` (or similar), assigning via `files[field] = file_obj` will collapse multiple files per field down to a single one. To keep multi-file support, iterate over `session_files.lists()` (or equivalent) and use `files.setlist(field, file_list)` so all files for each field are restored.
</issue_to_address>
### Comment 2
<location> `app/eventyay/static/cfp/js/formAutoSave.js:37-46` </location>
<code_context>
+
+ // Find all form fields using querySelectorAll for reliability
+ const textareas = form.querySelectorAll('textarea');
+ const inputs = form.querySelectorAll('input:not([type="hidden"]):not([type="submit"])');
+ const selects = form.querySelectorAll('select');
+
+ // Process all textareas
+ textareas.forEach((textarea) => {
+ const name = textarea.name;
+ if (name && name !== 'csrfmiddlewaretoken') {
+ formData[name] = textarea.value;
+ }
+ });
+
+ // Process all inputs
+ inputs.forEach((input) => {
+ const name = input.name;
+ const type = input.type;
+
+ if (!name || name === 'csrfmiddlewaretoken' || name === 'action') return;
+
+ if (type === 'checkbox') {
+ formData[name] = input.checked;
+ } else if (type === 'radio') {
</code_context>
<issue_to_address>
**issue:** Checkbox groups with the same name are not handled correctly in auto-save
The logic currently saves a single boolean per checkbox `name`, so groups with shared names overwrite each other and are all restored to the same value. For checkbox groups, you should instead persist an array of selected values (like multi-selects) and, on restore, set each checkbox’s `checked` state based on whether its value is in that array.
</issue_to_address>
### Comment 3
<location> `app/eventyay/static/cfp/js/formAutoSave.js:101-107` </location>
<code_context>
+ let allFieldsMatch = true;
+ let checkedFields = 0;
+
+ for (const [name, savedValue] of Object.entries(formData)) {
+ const elements = form.elements[name];
+ if (!elements) continue;
+
+ const element = elements.length !== undefined ? elements[0] : elements;
+ const currentValue = element.value || '';
+ const savedValueStr = String(savedValue || '');
+
+ checkedFields++;
</code_context>
<issue_to_address>
**issue (bug_risk):** Form matching logic for clearing sessionStorage doesn’t account for field types and groups
This logic assumes each form field is a single scalar value (`elements[0].value`), which breaks for grouped inputs. For radio/checkbox groups only the first element is inspected, so you can mis-detect mismatches. For checkboxes, you’re comparing `'on'` to a stored boolean, so `allFieldsMatch` will almost always be false and sessionStorage will rarely be cleared. Consider normalizing by field type (using `checked` for checkboxes, selected value for radios, arrays for multi-selects) to match the structure produced by `saveFormData`.
</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
+37
to
+46
| const inputs = form.querySelectorAll('input:not([type="hidden"]):not([type="submit"])'); | ||
| const selects = form.querySelectorAll('select'); | ||
|
|
||
| // Process all textareas | ||
| textareas.forEach((textarea) => { | ||
| const name = textarea.name; | ||
| if (name && name !== 'csrfmiddlewaretoken') { | ||
| formData[name] = textarea.value; | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
issue: Checkbox groups with the same name are not handled correctly in auto-save
The logic currently saves a single boolean per checkbox name, so groups with shared names overwrite each other and are all restored to the same value. For checkbox groups, you should instead persist an array of selected values (like multi-selects) and, on restore, set each checkbox’s checked state based on whether its value is in that array.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+75
to
+76
| # When clicking Back, the step's POST handler has already saved the data | ||
| # Now redirect to the previous step |
There was a problem hiding this comment.
The comment states "When clicking Back, the step's POST handler has already saved the data" but this is only partially true. Looking at the flow.py implementation, the data is only saved if the form is valid (line 281). If the form is invalid, no data is saved but the user is still redirected back. Consider updating this comment to reflect this nuance or ensuring the back button behavior is consistent with the comment.
Suggested change
| # When clicking Back, the step's POST handler has already saved the data | |
| # Now redirect to the previous step | |
| # When clicking Back, the step's POST handler has already processed the data | |
| # (and, if valid, saved it). Now redirect to the previous step. |
Comment on lines
+124
to
+160
| for (const [name, value] of Object.entries(formData)) { | ||
| const elements = form.elements[name]; | ||
| if (!elements) continue; | ||
|
|
||
| // Handle NodeList (radio buttons, checkboxes with same name) | ||
| if (elements.length > 1) { | ||
| for (let i = 0; i < elements.length; i++) { | ||
| const element = elements[i]; | ||
| if (element.type === 'checkbox') { | ||
| element.checked = value; | ||
| } else if (element.type === 'radio') { | ||
| element.checked = (element.value === value); | ||
| } | ||
| } | ||
| } else { | ||
| const element = elements.length !== undefined ? elements[0] : elements; | ||
| const type = element.type; | ||
|
|
||
| if (type === 'checkbox') { | ||
| element.checked = value; | ||
| } else if (type === 'radio') { | ||
| element.checked = (element.value === value); | ||
| } else if (element.tagName === 'SELECT') { | ||
| if (element.multiple && Array.isArray(value)) { | ||
| for (let i = 0; i < element.options.length; i++) { | ||
| element.options[i].selected = value.includes(element.options[i].value); | ||
| } | ||
| } else { | ||
| element.value = value; | ||
| } | ||
| } else if (element.tagName === 'TEXTAREA' || element.tagName === 'INPUT') { | ||
| // Always restore - sessionStorage takes precedence | ||
| // If we got this far, the matching logic already determined | ||
| // this is new data that should be restored | ||
| element.value = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
The code iterates through form.elements[name] which can be either a single element or a NodeList depending on whether multiple elements share the same name. The check 'elements.length !== undefined' is used to distinguish these cases, but this is fragile. NodeList and HTMLCollection both have a length property, but a single element does not. However, checking for 'undefined' specifically could fail if length exists but is 0. Consider using a more robust check like 'elements instanceof NodeList || elements instanceof RadioNodeList' or 'elements.tagName === undefined' to distinguish collections from single elements.
Comment on lines
+101
to
+139
| for (const [name, savedValue] of Object.entries(formData)) { | ||
| const elements = form.elements[name]; | ||
| if (!elements) continue; | ||
|
|
||
| const element = elements.length !== undefined ? elements[0] : elements; | ||
| const currentValue = element.value || ''; | ||
| const savedValueStr = String(savedValue || ''); | ||
|
|
||
| checkedFields++; | ||
| if (currentValue.trim() !== savedValueStr.trim()) { | ||
| allFieldsMatch = false; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // If all saved fields match current values AND we checked some fields, | ||
| // it means the form was successfully submitted - clear sessionStorage | ||
| if (allFieldsMatch && checkedFields > 0) { | ||
| clearFormData(); | ||
| return; | ||
| } | ||
|
|
||
| // Otherwise, restore from sessionStorage | ||
| for (const [name, value] of Object.entries(formData)) { | ||
| const elements = form.elements[name]; | ||
| if (!elements) continue; | ||
|
|
||
| // Handle NodeList (radio buttons, checkboxes with same name) | ||
| if (elements.length > 1) { | ||
| for (let i = 0; i < elements.length; i++) { | ||
| const element = elements[i]; | ||
| if (element.type === 'checkbox') { | ||
| element.checked = value; | ||
| } else if (element.type === 'radio') { | ||
| element.checked = (element.value === value); | ||
| } | ||
| } | ||
| } else { | ||
| const element = elements.length !== undefined ? elements[0] : elements; |
There was a problem hiding this comment.
The logic for checking if an element is a NodeList and accessing the first element is duplicated in lines 105 and 139. The pattern 'elements.length !== undefined ? elements[0] : elements' appears twice. Consider extracting this into a helper function to reduce duplication and improve maintainability.
Comment on lines
+101
to
+110
| for (const [name, savedValue] of Object.entries(formData)) { | ||
| const elements = form.elements[name]; | ||
| if (!elements) continue; | ||
|
|
||
| const element = elements.length !== undefined ? elements[0] : elements; | ||
| const currentValue = element.value || ''; | ||
| const savedValueStr = String(savedValue || ''); | ||
|
|
||
| checkedFields++; | ||
| if (currentValue.trim() !== savedValueStr.trim()) { |
There was a problem hiding this comment.
The comparison 'currentValue.trim() !== savedValueStr.trim()' assumes both values are strings with a trim() method. However, if the currentValue is from a checkbox or other non-text input, it might not have a trim() method. This could cause a runtime error. Consider adding type checks or handling non-string values appropriately.
| <i class="fa fa-info-circle"></i> {% translate "Previously uploaded files:" %} | ||
| <ul class="mb-0 mt-1"> | ||
| {% for field_name, filename in uploaded_files.items %} | ||
| <li><strong>{{ field_name|default:'' }}</strong>: {{ filename }} {% translate "(will be preserved if you don't upload a new file)" %}</li> |
There was a problem hiding this comment.
The field name is used directly in the template display without any sanitization or prettification. Field names like "id_image" or "proposal_file" will be displayed as-is to users. Consider either using the field's verbose_name/label from the form definition, or adding a filter to make the field name more user-friendly.
Suggested change
| <li><strong>{{ field_name|default:'' }}</strong>: {{ filename }} {% translate "(will be preserved if you don't upload a new file)" %}</li> | |
| <li><strong>{{ field_name|default:''|capfirst }}</strong>: {{ filename }} {% translate "(will be preserved if you don't upload a new file)" %}</li> |
Comment on lines
+95
to
+121
| // Check if the saved sessionStorage data matches the current form data | ||
| // If they match exactly, it means we successfully submitted and the server | ||
| // echoed back our data - in this case, clear sessionStorage | ||
| let allFieldsMatch = true; | ||
| let checkedFields = 0; | ||
|
|
||
| for (const [name, savedValue] of Object.entries(formData)) { | ||
| const elements = form.elements[name]; | ||
| if (!elements) continue; | ||
|
|
||
| const element = elements.length !== undefined ? elements[0] : elements; | ||
| const currentValue = element.value || ''; | ||
| const savedValueStr = String(savedValue || ''); | ||
|
|
||
| checkedFields++; | ||
| if (currentValue.trim() !== savedValueStr.trim()) { | ||
| allFieldsMatch = false; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // If all saved fields match current values AND we checked some fields, | ||
| // it means the form was successfully submitted - clear sessionStorage | ||
| if (allFieldsMatch && checkedFields > 0) { | ||
| clearFormData(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The logic here checks if allFieldsMatch and clears sessionStorage if true. However, this assumes that a match between saved and current values means successful submission. This could lead to false positives in edge cases where the user manually enters the exact same values that were in sessionStorage, then navigates back before submitting. Consider adding an additional indicator (e.g., a flag in sessionStorage) to explicitly track successful form submission.
hongquan
requested changes
Dec 20, 2025
| {% include "cfp/includes/forms_header.html" %} | ||
| {% compress js %} | ||
| <script defer src="{% static "cfp/js/proposalTabTitles.js" %}"></script> | ||
| <script defer src="{% static "cfp/js/formAutoSave.js" %}"></script> |
Member
There was a problem hiding this comment.
Please use different quote style when nesting.
| * This ensures data is not lost when users use the browser back button | ||
| */ | ||
|
|
||
| (function() { |
Member
There was a problem hiding this comment.
This style of JS code (wrap the whole code in an anonymous function) is outdated and verbose.
Please use "<script type='module'>".
Member
Author
There was a problem hiding this comment.
Probably I'm outdated with respect to js ;-)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes: #1391
After going to the second page and returning, and when an image has been uploaded on the first page, we see:
Summary by Sourcery
Preserve previously uploaded files and their metadata across CFP submission steps and display them in the submission UI.
Enhancements:
Summary by Sourcery
Preserve CFP proposal form data and uploaded files when navigating between submission steps or using browser navigation, and surface preserved file info in the UI.
New Features:
Bug Fixes:
Enhancements: