fix: add guard to prevent bulk export across multiple forms in admin … - #30
Merged
Merged
Conversation
Contributor
Reviewer's GuideAdds a server-side guard to the SavedFormDataEntry admin bulk export action so it rejects querysets spanning multiple form entries with a localized error message and redirect, while leaving single-form and inline export behavior unchanged and covered by new regression tests. Sequence diagram for guarded bulk export action in SavedFormDataEntry adminsequenceDiagram
actor AdminUser
participant DjangoAdmin as DjangoAdminChangelist
participant SavedAdmin as SavedFormDataEntryAdmin
participant Messages as DjangoMessages
participant Exporter as SuperExportData
AdminUser->>DjangoAdmin: Select rows and choose export_data
DjangoAdmin->>SavedAdmin: export_data(request, queryset)
SavedAdmin->>SavedAdmin: form_entry_ids = queryset.values_list(...).distinct()
alt Multiple form_entry_ids
SavedAdmin->>Messages: error(request, "Export across multiple forms is not supported ...")
SavedAdmin-->>DjangoAdmin: HttpResponseRedirect(request.get_full_path())
DjangoAdmin-->>AdminUser: Redirect back to filtered changelist
else Single or zero form_entry_ids
SavedAdmin->>Exporter: export_data(request, queryset)
Exporter-->>SavedAdmin: HttpResponse(response)
SavedAdmin-->>DjangoAdmin: HttpResponse(response)
DjangoAdmin-->>AdminUser: Download CSV/XLS
end
rect rgb(230,230,230)
note over SavedAdmin: Inline export path
AdminUser->>DjangoAdmin: Click inline export button
DjangoAdmin->>SavedAdmin: export_for_form_entry(request)
SavedAdmin->>SavedAdmin: queryset = filter(form_entry_id=form_entry_id)
SavedAdmin->>SavedAdmin: export_data(request, queryset)
SavedAdmin->>Exporter: export_data(request, single_form_queryset)
Exporter-->>SavedAdmin: HttpResponse(response)
SavedAdmin-->>DjangoAdmin: HttpResponse(response)
DjangoAdmin-->>AdminUser: Download CSV/XLS
end
Class diagram for SavedFormDataEntry admin export_data overrideclassDiagram
class BaseSavedFormDataEntryAdmin {
+export_data(request, queryset) HttpResponse
}
class SavedFormDataEntryAdminIntegrationMixin {
+export_for_form_entry(request) HttpResponse
+export_data(request, queryset) HttpResponse
+_parse_json_field(raw) dict
}
class SavedFormDataEntryAdmin {
}
class MessagesFramework {
+error(request, message) None
}
class HttpResponseRedirect {
+url str
}
BaseSavedFormDataEntryAdmin <|-- SavedFormDataEntryAdmin
SavedFormDataEntryAdminIntegrationMixin <|-- SavedFormDataEntryAdmin
SavedFormDataEntryAdminIntegrationMixin ..> BaseSavedFormDataEntryAdmin : calls_super_export_data
SavedFormDataEntryAdminIntegrationMixin ..> MessagesFramework : uses
SavedFormDataEntryAdminIntegrationMixin ..> HttpResponseRedirect : returns
note for SavedFormDataEntryAdminIntegrationMixin "export_data enforces single-form querysets; on mixed forms it sends an error message and returns HttpResponseRedirect(request.get_full_path()). On single or zero forms it delegates to super().export_data unchanged. export_for_form_entry always builds a single-form queryset before calling export_data."
File-Level Changes
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 1 issue, and left some high level feedback:
- In the new
export_dataoverride, you only need to know whether there is more than one distinctform_entry_id, so you can avoid materializing the fullform_entry_idslist by using something likedistinct().values_list(..., flat=True)[:2](or.count()), which will be more efficient on large querysets.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new `export_data` override, you only need to know whether there is more than one distinct `form_entry_id`, so you can avoid materializing the full `form_entry_ids` list by using something like `distinct().values_list(..., flat=True)[:2]` (or `.count()`), which will be more efficient on large querysets.
## Individual Comments
### Comment 1
<location path="tests/admin/test_db_store_export_action.py" line_range="450-452" />
<code_context>
+
+ assert response.status_code == 302
+ assert captured["calls"] == 0
+ emitted = [str(m) for m in request._messages]
+ assert len(emitted) == 1
+ assert "multiple forms" in emitted[0].lower()
+
+ def test_mixed_form_redirect_preserves_query_string(
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the assertion on the message level/severity, not only its text
The test currently asserts only the message text. Since the code uses `messages.error`, it would be helpful to also assert the message level (e.g. `emitted_messages = list(request._messages); assert emitted_messages[0].level == messages.ERROR`) to prevent future refactors from silently changing this to a non-error message while the test still passes.
</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
+450
to
+452
| emitted = [str(m) for m in request._messages] | ||
| assert len(emitted) == 1 | ||
| assert "multiple forms" in emitted[0].lower() |
Contributor
There was a problem hiding this comment.
suggestion (testing): Strengthen the assertion on the message level/severity, not only its text
The test currently asserts only the message text. Since the code uses messages.error, it would be helpful to also assert the message level (e.g. emitted_messages = list(request._messages); assert emitted_messages[0].level == messages.ERROR) to prevent future refactors from silently changing this to a non-error message while the test still passes.
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
1 participant
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.
Summary by Sourcery
Guard the db_store admin bulk export action against mixed-form querysets and surface a translated error instead of generating misleading multi-form exports.
New Features:
Enhancements:
Tests:
Chores: