Feature kpoland refactor django web views file by klpoland · Pull Request #270 · spectrumx/sds-code

klpoland · 2026-03-27T20:30:02Z

Migrating views into new files and aligning with urls.py. Keeping a views_deprecated.py file for now to check accuracy.

semanticdiff-com · 2026-03-27T20:30:05Z

Review changes with

Changed Files

File	Status
gateway/sds_gateway/users/views/__init__.py	0% smaller
gateway/sds_gateway/users/views/api_keys.py	0% smaller
gateway/sds_gateway/users/views/captures.py	0% smaller
gateway/sds_gateway/users/views/datasets.py	0% smaller
gateway/sds_gateway/users/views/downloads.py	0% smaller
gateway/sds_gateway/users/views/files.py	0% smaller
gateway/sds_gateway/users/views/share_groups.py	0% smaller
gateway/sds_gateway/users/views/sharing.py	0% smaller
gateway/sds_gateway/users/views/special_pages.py	0% smaller
gateway/sds_gateway/users/views/uploads.py	0% smaller
gateway/sds_gateway/users/views/user_profile.py	0% smaller
gateway/sds_gateway/users/views/utilities.py	0% smaller
gateway/sds_gateway/users/views_deprecated.py	0% smaller

lucaspar · 2026-03-27T20:48:02Z

makes sense at a high level; after copilot's comments, run just hooks and just test, fix any issues, then we can do a squash merge.

Copilot

Pull request overview

This PR refactors the Django users app views by splitting a large views module into multiple focused modules, while keeping backwards-compatible imports via users/views/__init__.py and retaining a deprecated views file for verification.

Changes:

Split user-facing views into dedicated modules (api_keys, captures, datasets, downloads, files, sharing, etc.).
Add/maintain re-export surface in users/views/__init__.py to preserve existing import paths.
Introduce/retain generic HTML fragment rendering endpoint and other utility endpoints in their own module.

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`gateway/sds_gateway/users/views/__init__.py`	Central re-exports to keep legacy imports working after module split.
`gateway/sds_gateway/users/views/api_keys.py`	API key management views/utilities moved into their own module.
`gateway/sds_gateway/users/views/captures.py`	Capture list + API search + keyword autocomplete views moved/refactored.
`gateway/sds_gateway/users/views/datasets.py`	Dataset creation/editing, listing, details, publish, versioning, and published-dataset search.
`gateway/sds_gateway/users/views/downloads.py`	Temporary zip download and item download request views.
`gateway/sds_gateway/users/views/files.py`	File list/detail/download/preview and files UI views.
`gateway/sds_gateway/users/views/share_groups.py`	Share group management views separated out.
`gateway/sds_gateway/users/views/sharing.py`	Generic item sharing endpoint and helpers separated out.
`gateway/sds_gateway/users/views/special_pages.py`	Home page + special competition page views separated out.
`gateway/sds_gateway/users/views/uploads.py`	Upload/capture creation flow separated out.
`gateway/sds_gateway/users/views/user_profile.py`	User profile detail/update/redirect views separated out.
`gateway/sds_gateway/users/views/utilities.py`	Generic server-side fragment rendering endpoint separated out.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-27T20:49:13Z

gateway/sds_gateway/users/views/datasets.py

+    matching_dataset_ids = {
+        capture.dataset_id
+        for capture in filtered_captures
+        if capture.dataset_id is not None
+    }


This mapping back to datasets uses capture.dataset_id, which will be None for captures associated via the M2M Capture.datasets relationship. That will cause matching datasets to be dropped when applying frequency filters. Consider deriving dataset IDs/UUIDs from capture.datasets (and the FK for backward compatibility) instead of relying on dataset_id.

Copilot · 2026-03-27T20:49:13Z

gateway/sds_gateway/users/views/files.py

+
+    def get(self, request, *args, **kwargs) -> HttpResponse:
+        # Get query parameters
+        page = int(request.GET.get("page", 1))


page is coerced with int(request.GET.get('page', 1)). If a non-numeric value is provided (e.g. ?page=abc), this will raise ValueError before pagination handling and return a 500. Consider leaving it as a string and using paginator.get_page(...), or wrapping the int() conversion in a try/except ValueError and falling back to page 1.

Suggested change

page = int(request.GET.get("page", 1))

page_param = request.GET.get("page", 1)

try:

page = int(page_param)

except (TypeError, ValueError):

page = 1

Copilot · 2026-03-27T20:49:14Z

gateway/sds_gateway/users/views/downloads.py

+            with file_path.open("rb") as f:
+                file_content = f.read()
+                response = HttpResponse(file_content, content_type="application/zip")
+                response["Content-Disposition"] = (
+                    f'attachment; filename="{temp_zip.filename}"'
+                )
+                response["Content-Length"] = file_size


This view reads the entire zip into memory (file_content = f.read()) before returning it. Temporary zip downloads can be large, so this can cause high memory usage and slow response times. Use Django’s FileResponse/streaming response to stream the file from disk instead of buffering it fully in RAM.

bit of a tangent, but this might actually be a good one to address

Copilot · 2026-03-27T20:49:14Z

gateway/sds_gateway/users/views/captures.py

+        """Extract and return request parameters for HTML view."""
+        return {
+            "page": int(request.GET.get("page", 1)),
+            "sort_by": request.GET.get("sort_by", "created_at"),
+            "sort_order": request.GET.get("sort_order", "desc"),
+            "search": request.GET.get("search", ""),
+            "date_start": request.GET.get("date_start", ""),
+            "date_end": request.GET.get("date_end", ""),
+            "cap_type": request.GET.get("capture_type", ""),
+            "min_freq": request.GET.get("min_freq", ""),
+            "max_freq": request.GET.get("max_freq", ""),
+            "items_per_page": min(
+                int(request.GET.get("items_per_page", self.default_items_per_page)),
+                self.max_items_per_page,
+            ),
+        }


page/items_per_page are coerced with int(...) directly from query params. If a non-numeric value is provided (e.g. ?page=abc or ?items_per_page=foo), this will raise ValueError before the paginator fallback logic and return a 500. Consider using paginator.get_page(...) and validating ints with try/except ValueError (clamping to defaults on failure).

Copilot · 2026-03-27T20:49:14Z

gateway/sds_gateway/users/views/utilities.py

+# SECURITY MODEL:
+# - Only templates in users/components/ directory are allowed (enforced by prefix check)
+# - Context data is provided by the client, not pulled from the database
+# - All data is rendered through Django templates with automatic HTML escaping
+# - CSRF protection is still enforced by Django middleware
+# - No sensitive server-side data is exposed - only client-provided data is rendered


The security note claims “Django's automatic HTML escaping prevents XSS”, but this endpoint renders templates with fully client-provided context and some components use the safe filter (e.g., templates/users/components/table_rows.html renders cell.html|safe). That means a caller can inject raw HTML/JS into the rendered fragment. Consider restricting this endpoint to an explicit allowlist of templates that never use |safe with client-controlled values, and/or sanitizing/escaping specific context fields before rendering (or removing the XSS claim from the docstring).

Copilot · 2026-03-27T20:49:14Z

gateway/sds_gateway/users/views/user_profile.py

+class UserDetailView(Auth0LoginRequiredMixin, DetailView):  # pyright: ignore[reportMissingTypeArgument]
+    model = User
+    slug_field = "id"
+    slug_url_arg = "id"


slug_url_arg is not a Django DetailView attribute (the correct attribute name is slug_url_kwarg). As written, this configuration is ignored and can be misleading; either remove the slug configuration (since the URL uses <int:pk>) or rename it to slug_url_kwarg and update the URL pattern accordingly.

Suggested change

slug_url_arg = "id"

C-3PO seems correct https://github.com/django/django/blob/main/django/views/generic/detail.py#L17

Copilot · 2026-03-27T20:49:15Z

gateway/sds_gateway/users/views/datasets.py

+    return datasets
+
+
+class SearchPublishedDatasetsView(Auth0LoginRequiredMixin, View):


SearchPublishedDatasetsView is described as “public, no auth required”, and it’s linked from the public home page, but it inherits Auth0LoginRequiredMixin which forces a login redirect. If this page is intended to be publicly browsable, drop the login-required mixin (and keep the serializer context logic for anonymous users).

Suggested change

class SearchPublishedDatasetsView(Auth0LoginRequiredMixin, View):

class SearchPublishedDatasetsView(View):

added last month, see if we need to update the mixin or the comment

9876d5d

Copilot · 2026-03-27T20:49:15Z

gateway/sds_gateway/users/views/datasets.py

+    # Get all captures for these datasets and convert to list
+    captures_qs = Capture.objects.filter(
+        dataset__uuid__in=dataset_uuids, is_deleted=False
+    )


Frequency filtering queries captures via the deprecated FK (Capture.dataset) only. Since dataset composition is handled via the M2M Capture.datasets / Dataset.captures relationship (expand/contract), this will miss captures added through the current UI and make frequency filters return empty/incorrect results. Use a query that includes both relationships (e.g., Q(datasets__uuid__in=...) | Q(dataset__uuid__in=...)), or reuse relationship_utils.get_dataset_captures()/get_dataset_files_including_captures() patterns.

klpoland added 5 commits March 27, 2026 13:09

break out views.py into multiple files in views folder, relink urls

50f8931

add publishing views to new files

8bc5eea

move dataset versioning view

db99f66

align datasets versioning views, bug fixes

68965ba

align updated views

1621a69

klpoland requested a review from lucaspar March 27, 2026 20:30

klpoland self-assigned this Mar 27, 2026

klpoland added refactoring General code improvements gateway Gateway component labels Mar 27, 2026

lucaspar requested a review from Copilot March 27, 2026 20:42

Copilot started reviewing on behalf of lucaspar March 27, 2026 20:43 View session

Copilot AI reviewed Mar 27, 2026

View reviewed changes

-        page = int(request.GET.get("page", 1))
+        page_param = request.GET.get("page", 1)
+        try:
+            page = int(page_param)
+        except (TypeError, ValueError):
+            page = 1

		return datasets


		class SearchPublishedDatasetsView(Auth0LoginRequiredMixin, View):

	class SearchPublishedDatasetsView(Auth0LoginRequiredMixin, View):
	class SearchPublishedDatasetsView(View):

Conversation

klpoland commented Mar 27, 2026

Uh oh!

semanticdiff-com bot commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucaspar commented Mar 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

lucaspar Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

lucaspar Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

lucaspar Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

semanticdiff-com bot commented Mar 27, 2026 •

edited

Loading

lucaspar Mar 27, 2026 •

edited

Loading