added backup download page for student data competition dataset download by david-pettifor-nd · Pull Request #241 · spectrumx/sds-code

david-pettifor-nd · 2025-12-21T20:54:07Z

Screenshot:

Must add public URL for S3 bucket file to the django.env file.

Backup page is at /spx-dac-dataset-alt/ URL.

semanticdiff-com · 2025-12-21T20:54:10Z

Review changes with

Changed Files

File	Status
gateway/sds_gateway/users/views.py	1% smaller
gateway/.envs/example/django.env	Unsupported file format
gateway/config/settings/base.py	0% smaller
gateway/config/urls.py	0% smaller
gateway/sds_gateway/templates/pages/spx_dac_dataset_alt.html	0% smaller

…PX DAC dataset

Copilot

Pull request overview

This PR adds a backup download page for the NSF SpectrumX Data and Algorithm Competition (SpX-DAC) dataset. The page provides students with instructions to download the competition dataset via the SpectrumX Python library, automatically shares the dataset with authenticated users, and includes API key generation functionality.

Key changes:

New view class SPXDACDatasetAltView with GET and POST methods for displaying the page and generating API keys
Comprehensive HTML template with step-by-step instructions, Python code examples, and optional direct S3 download
Configuration settings for dataset ID and S3 bucket URL

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 17 comments.

Show a summary per file

File	Description
`gateway/sds_gateway/users/views.py`	Implements SPXDACDatasetAltView with automatic dataset sharing on GET and API key generation on POST
`gateway/sds_gateway/templates/pages/spx_dac_dataset_alt.html`	Complete HTML template with interactive JavaScript for API key generation and code examples
`gateway/config/urls.py`	Registers the new view at `/spx-dac/` URL path
`gateway/config/settings/base.py`	Adds configuration for dataset ID and S3 URL environment variables
`gateway/.envs/example/django.env`	Documents the new environment variables for dataset configuration

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

Copilot · 2025-12-22T02:04:01Z

+                    # Check if permission already exists
+                    existing_permission = UserSharePermission.objects.filter(
+                        owner=dataset.owner,
+                        shared_with=request.user,
+                        item_type=ItemType.DATASET,
+                        item_uuid=dataset_uuid,
+                        is_deleted=False,
+                    ).first()
+
+                    if not existing_permission:
+                        # Create share permission with VIEWER role
+                        UserSharePermission.objects.create(
+                            owner=dataset.owner,
+                            shared_with=request.user,
+                            item_type=ItemType.DATASET,
+                            item_uuid=dataset_uuid,
+                            message="Automatically shared for NSF SpectrumX Data and Algorithm Competition (SpX-DAC)",
+                            permission_level=PermissionLevel.VIEWER,
+                            is_enabled=True,
+                        )
+                        log.info(f"Automatically shared SpX-DAC dataset with user {request.user.email}")
+                    elif not existing_permission.is_enabled:
+                        # Re-enable if it was previously disabled
+                        existing_permission.is_enabled = True
+                        existing_permission.save()


The check for existing share permission and creation of a new one (lines 4127-4145) is not atomic. There's a potential race condition where two concurrent requests could both pass the 'existing_permission' check and attempt to create duplicate permissions. Consider using get_or_create() or implementing a database-level unique constraint to prevent duplicates.

Suggested change

# Check if permission already exists

existing_permission = UserSharePermission.objects.filter(

owner=dataset.owner,

shared_with=request.user,

item_type=ItemType.DATASET,

item_uuid=dataset_uuid,

is_deleted=False,

).first()

if not existing_permission:

# Create share permission with VIEWER role

UserSharePermission.objects.create(

owner=dataset.owner,

shared_with=request.user,

item_type=ItemType.DATASET,

item_uuid=dataset_uuid,

message="Automatically shared for NSF SpectrumX Data and Algorithm Competition (SpX-DAC)",

permission_level=PermissionLevel.VIEWER,

is_enabled=True,

)

log.info(f"Automatically shared SpX-DAC dataset with user {request.user.email}")

elif not existing_permission.is_enabled:

# Re-enable if it was previously disabled

existing_permission.is_enabled = True

existing_permission.save()

# Ensure share permission exists atomically

try:

permission, created = UserSharePermission.objects.get_or_create(

owner=dataset.owner,

shared_with=request.user,

item_type=ItemType.DATASET,

item_uuid=dataset_uuid,

is_deleted=False,

defaults={

"message": "Automatically shared for NSF SpectrumX Data and Algorithm Competition (SpX-DAC)",

"permission_level": PermissionLevel.VIEWER,

"is_enabled": True,

},

)

except IntegrityError:

# In case of a race condition with a unique constraint, fall back to fetching existing permission

permission = UserSharePermission.objects.filter(

owner=dataset.owner,

shared_with=request.user,

item_type=ItemType.DATASET,

item_uuid=dataset_uuid,

is_deleted=False,

).first()

created = False

if created:

log.info(f"Automatically shared SpX-DAC dataset with user {request.user.email}")

elif permission and not permission.is_enabled:

# Re-enable if it was previously disabled

permission.is_enabled = True

permission.save()

Copilot · 2025-12-22T02:04:01Z

    path("users/", include("sds_gateway.users.urls", namespace="users")),
    path("accounts/", include("allauth.urls")),
+    # SpectrumX DAC Dataset alternative download
+    path("spx-dac/", spx_dac_dataset_alt_view, name="spx_dac_dataset_alt"),


The URL path in urls.py is "spx-dac/" but the PR description states the backup page is at "/spx-dac-dataset-alt/". This inconsistency means the URL won't match the documentation. Consider updating the path to match the description or updating the documentation to match the implementation.

Suggested change

path("spx-dac/", spx_dac_dataset_alt_view, name="spx_dac_dataset_alt"),

path("spx-dac-dataset-alt/", spx_dac_dataset_alt_view, name="spx_dac_dataset_alt"),

Copilot · 2025-12-22T02:04:01Z

+
+        context = {
+            "s3_bucket_url": settings.SPX_DAC_DATASET_S3_URL,
+            "dataset_id": dataset_id or "458c3f72-8d7e-49cc-9be3-ed0b0cd7e03d",


The fallback dataset_id hardcoded value "458c3f72-8d7e-49cc-9be3-ed0b0cd7e03d" is used both in the context dictionary (line 4157) and as the default in settings (line 77 of base.py). If the environment variable is not set but is an empty string, the fallback on line 4157 will never be used because the condition checks for falsiness. Consider using 'or' operator or checking explicitly for None.

Suggested change

"dataset_id": dataset_id or "458c3f72-8d7e-49cc-9be3-ed0b0cd7e03d",

"dataset_id": dataset_id,

Copilot · 2025-12-22T02:04:02Z

+
+# when in dry-run (default), no changes are made to the SDS or the local filesystem
+# to enable the changes, set dry_run to False, as in:
+client.dry_run = False  # ⚠️ This is required to actually download files!


The comment says "⚠️ This is required to actually download files!" but uses an emoji warning symbol. While this is visually clear, consider using a text-based comment format for better accessibility and to ensure it displays correctly across all text editors and systems.

Suggested change

client.dry_run = False # ⚠️ This is required to actually download files!

client.dry_run = False # WARNING: This is required to actually download files!

Copilot · 2025-12-22T02:04:02Z

+# S3 bucket URL for the SpectrumX DAC Dataset alternative download page
+# SPX_DAC_DATASET_S3_URL=https://your-s3-bucket-url.com/path/to/dataset
+# Dataset ID for the SpX-DAC competition dataset


The environment variable examples are commented out, but there's no indication in the comments whether SPX_DAC_DATASET_S3_URL is optional or required for the feature to work. Consider adding a comment explaining that SPX_DAC_DATASET_S3_URL enables the direct download fallback option, while SPX_DAC_DATASET_ID has a default value.

Suggested change

# S3 bucket URL for the SpectrumX DAC Dataset alternative download page

# SPX_DAC_DATASET_S3_URL=https://your-s3-bucket-url.com/path/to/dataset

# Dataset ID for the SpX-DAC competition dataset

# Optional: S3 bucket URL for the SpectrumX DAC Dataset alternative download page.

# When set, this enables the direct download fallback option.

# SPX_DAC_DATASET_S3_URL=https://your-s3-bucket-url.com/path/to/dataset

# Dataset ID for the SpX-DAC competition dataset. This has a default value in the

# application; set this only if you need to override the default dataset ID.

Copilot · 2025-12-22T02:04:04Z

+    """View for the SpectrumX Student Data Competition page."""
+
+    template_name = "pages/spx_dac_dataset_alt.html"
+
+    def get(self, request, *args, **kwargs):
+        """Display the student data competition page and automatically share the dataset."""
+        dataset_id = settings.SPX_DAC_DATASET_ID
+        if not dataset_id:
+            log.warning("SPX_DAC_DATASET_ID not configured")
+        else:
+            try:
+                dataset_uuid = UUID(dataset_id)
+                # Get the dataset to find its owner
+                try:
+                    dataset = Dataset.objects.get(uuid=dataset_uuid, is_deleted=False)
+                except Dataset.DoesNotExist:
+                    log.warning(f"SpX-DAC dataset {dataset_id} not found")
+                    dataset = None
+
+                # Check if user is already the owner
+                if dataset and dataset.owner != request.user:
+                    # Check if permission already exists
+                    existing_permission = UserSharePermission.objects.filter(
+                        owner=dataset.owner,
+                        shared_with=request.user,
+                        item_type=ItemType.DATASET,
+                        item_uuid=dataset_uuid,
+                        is_deleted=False,
+                    ).first()
+
+                    if not existing_permission:
+                        # Create share permission with VIEWER role
+                        UserSharePermission.objects.create(
+                            owner=dataset.owner,
+                            shared_with=request.user,
+                            item_type=ItemType.DATASET,
+                            item_uuid=dataset_uuid,
+                            message="Automatically shared for NSF SpectrumX Data and Algorithm Competition (SpX-DAC)",
+                            permission_level=PermissionLevel.VIEWER,
+                            is_enabled=True,
+                        )
+                        log.info(f"Automatically shared SpX-DAC dataset with user {request.user.email}")
+                    elif not existing_permission.is_enabled:
+                        # Re-enable if it was previously disabled
+                        existing_permission.is_enabled = True
+                        existing_permission.save()
+                        log.info(f"Re-enabled SpX-DAC dataset share for user {request.user.email}")


The log messages use inconsistent terminology: "SpX-DAC" (line 4121, 4146, 4151) vs "SpectrumX Student Data Competition" (line 4105) vs "NSF SpectrumX Data and Algorithm Competition" (line 4142). Consider standardizing on one abbreviation or full name throughout the code for clarity and searchability.

Copilot · 2025-12-22T02:04:04Z

+    def get(self, request, *args, **kwargs):
+        """Display the student data competition page and automatically share the dataset."""
+        dataset_id = settings.SPX_DAC_DATASET_ID
+        if not dataset_id:
+            log.warning("SPX_DAC_DATASET_ID not configured")
+        else:
+            try:
+                dataset_uuid = UUID(dataset_id)
+                # Get the dataset to find its owner
+                try:
+                    dataset = Dataset.objects.get(uuid=dataset_uuid, is_deleted=False)
+                except Dataset.DoesNotExist:
+                    log.warning(f"SpX-DAC dataset {dataset_id} not found")
+                    dataset = None
+
+                # Check if user is already the owner
+                if dataset and dataset.owner != request.user:
+                    # Check if permission already exists
+                    existing_permission = UserSharePermission.objects.filter(
+                        owner=dataset.owner,
+                        shared_with=request.user,
+                        item_type=ItemType.DATASET,
+                        item_uuid=dataset_uuid,
+                        is_deleted=False,
+                    ).first()
+
+                    if not existing_permission:
+                        # Create share permission with VIEWER role
+                        UserSharePermission.objects.create(
+                            owner=dataset.owner,
+                            shared_with=request.user,
+                            item_type=ItemType.DATASET,
+                            item_uuid=dataset_uuid,
+                            message="Automatically shared for NSF SpectrumX Data and Algorithm Competition (SpX-DAC)",
+                            permission_level=PermissionLevel.VIEWER,
+                            is_enabled=True,
+                        )
+                        log.info(f"Automatically shared SpX-DAC dataset with user {request.user.email}")
+                    elif not existing_permission.is_enabled:
+                        # Re-enable if it was previously disabled
+                        existing_permission.is_enabled = True
+                        existing_permission.save()
+                        log.info(f"Re-enabled SpX-DAC dataset share for user {request.user.email}")


The view automatically shares the dataset with any authenticated user who visits the page. This side-effect in a GET request violates REST principles and may have unintended consequences. Consider requiring an explicit action (e.g., a button click in the UI that makes a POST request) before sharing the dataset, or clearly document this behavior to avoid confusion.

Copilot · 2025-12-22T02:04:04Z

+        try:
+            # Create an API key for the user
+            _, raw_key = UserAPIKey.objects.create_key(
+                name=api_key_name,
+                description=api_key_description,
+                user=request.user,
+                source=KeySources.SDSWebUI,
+                expiry_date=None,
+            )
+            return JsonResponse({"success": True, "api_key": raw_key})
+        except Exception as e:  # noqa: BLE001
+            log.exception("Error generating API key for student competition")
+            return JsonResponse(
+                {"success": False, "error": "Failed to generate API key. Please try again."},
+                status=500,
+            )


The broad exception handler catches all exceptions but doesn't differentiate between different error types. Consider catching specific exceptions (e.g., ValidationError from UserAPIKey.objects.create_key) to provide more specific error messages to users and better debugging information.

Copilot · 2025-12-22T02:04:04Z

+            function updatePythonCode(apiKey) {
+                const codeElement = document.getElementById('python-code-example');
+                if (codeElement) {
+                    const currentCode = codeElement.textContent;
+                    const updatedCode = currentCode.replace(
+                        /env_config=\{"SDS_SECRET_TOKEN": "YOUR_API_KEY"\}/,
+                        `env_config={"SDS_SECRET_TOKEN": "${apiKey}"}`


The JavaScript code replaces the API key in the Python example using a simple string replacement with a regex pattern. If the API key contains special regex characters or characters that need escaping in a string literal, this could produce invalid Python code. Consider using a more robust replacement method or escaping the API key appropriately.

Suggested change

function updatePythonCode(apiKey) {

const codeElement = document.getElementById('python-code-example');

if (codeElement) {

const currentCode = codeElement.textContent;

const updatedCode = currentCode.replace(

/env_config=\{"SDS_SECRET_TOKEN": "YOUR_API_KEY"\}/,

`env_config={"SDS_SECRET_TOKEN": "${apiKey}"}`

function escapeForPythonDoubleQuotedString(value) {

// Escape backslashes and double quotes for safe insertion into a Python double-quoted string

return value.replace(/\\/g, '\\\\').replace(/"/g, '\\"');

}

function updatePythonCode(apiKey) {

const codeElement = document.getElementById('python-code-example');

if (codeElement) {

const currentCode = codeElement.textContent;

const placeholder = 'env_config={"SDS_SECRET_TOKEN": "YOUR_API_KEY"}';

const escapedApiKey = escapeForPythonDoubleQuotedString(apiKey);

const updatedCode = currentCode.replace(

placeholder,

`env_config={"SDS_SECRET_TOKEN": "${escapedApiKey}"}`

Copilot · 2025-12-22T02:04:04Z

+# SpectrumX DAC Dataset S3 URL
+SPX_DAC_DATASET_S3_URL: str | None = env.str("SPX_DAC_DATASET_S3_URL", default=None)
+# SpectrumX DAC Dataset ID
+SPX_DAC_DATASET_ID: str | None = env.str("SPX_DAC_DATASET_ID", default="458c3f72-8d7e-49cc-9be3-ed0b0cd7e03d")


The SPX_DAC_DATASET_ID setting uses env.str() with a default value, but if an empty string is set in the environment, it will be used instead of the default. Consider using a custom helper or checking for empty strings to ensure the default is used when appropriate, similar to how SPX_DAC_DATASET_S3_URL is handled with default=None.

Suggested change

SPX_DAC_DATASET_ID: str | None = env.str("SPX_DAC_DATASET_ID", default="458c3f72-8d7e-49cc-9be3-ed0b0cd7e03d")

_spx_dac_dataset_id_env: str | None = env.str("SPX_DAC_DATASET_ID", default="")

SPX_DAC_DATASET_ID: str | None = (

_spx_dac_dataset_id_env or "458c3f72-8d7e-49cc-9be3-ed0b0cd7e03d"

)

added backup download page for student data competition dataset download

b75155d

david-pettifor-nd requested a review from lucaspar December 21, 2025 20:54

lucaspar reviewed Dec 21, 2025

View reviewed changes

Comment thread gateway/.envs/example/django.env

restored example django envs

60fb238

david-pettifor-nd requested a review from creinkin December 21, 2025 22:49

david-pettifor-nd added 2 commits December 21, 2025 19:17

added instructional guide with token generation and auto-sharing of S…

452326e

…PX DAC dataset

updated alt download section

ff2fba0

lucaspar assigned david-pettifor-nd Dec 22, 2025

lucaspar added documentation Improvements or additions to documentation feature New feature or request gateway Gateway component labels Dec 22, 2025

lucaspar requested a review from Copilot December 22, 2025 01:58

Copilot started reviewing on behalf of lucaspar December 22, 2025 01:58 View session

Copilot AI reviewed Dec 22, 2025

View reviewed changes

gwy: auto pre-commit fixes

b427068

lucaspar merged commit b427068 into master Dec 22, 2025

lucaspar deleted the feature-dp-alt-student-data-competition-download-page branch December 22, 2025 02:29

	path("spx-dac/", spx_dac_dataset_alt_view, name="spx_dac_dataset_alt"),
	path("spx-dac-dataset-alt/", spx_dac_dataset_alt_view, name="spx_dac_dataset_alt"),

	"dataset_id": dataset_id or "458c3f72-8d7e-49cc-9be3-ed0b0cd7e03d",
	"dataset_id": dataset_id,

	client.dry_run = False # ⚠️ This is required to actually download files!
	client.dry_run = False # WARNING: This is required to actually download files!

-SPX_DAC_DATASET_ID: str | None = env.str("SPX_DAC_DATASET_ID", default="458c3f72-8d7e-49cc-9be3-ed0b0cd7e03d")
+_spx_dac_dataset_id_env: str | None = env.str("SPX_DAC_DATASET_ID", default="")
+SPX_DAC_DATASET_ID: str | None = (
+    _spx_dac_dataset_id_env or "458c3f72-8d7e-49cc-9be3-ed0b0cd7e03d"
+)

Uh oh!

Conversation

david-pettifor-nd commented Dec 21, 2025

Uh oh!

semanticdiff-com Bot commented Dec 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

semanticdiff-com Bot commented Dec 21, 2025 •

edited

Loading