Gateway branding and other front-end improvements#257
Gateway branding and other front-end improvements#257
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds customizable branding to the SDS Gateway to distinguish different deployment instances, along with front-end improvements and a critical debouncing fix for capture search. The branding system allows operators to configure institution names, site identifiers, and optional branding images through environment variables, which are then displayed in the navbar, page titles, and homepage.
Changes:
- Fixed search debouncing using AbortController to cancel stale requests and prevent race conditions
- Added branding system with configurable site names, institution names, and optional branding images
- Improved UI consistency across captures and datasets pages with better visual hierarchy
- Added vertical dividers in navbar and restricted authenticated-only links to authenticated users
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| gateway/sds_gateway/context_processors.py | Added branding context processor to expose branding settings to all templates |
| gateway/sds_gateway/test_context_processors.py | Added tests for the new branding context processor |
| gateway/config/settings/base.py | Defined branding settings and helper function to resolve brand image paths |
| gateway/config/settings/production.py | Added SDS_SITE_FQDN to ALLOWED_HOSTS |
| gateway/config/settings/local.py | Added SDS_SITE_FQDN to ALLOWED_HOSTS |
| gateway/.envs/example/django.env | Added example branding environment variables for development |
| gateway/.envs/example/django.prod-example.env | Added example branding environment variables for production |
| gateway/sds_gateway/templates/base.html | Updated navbar with branding, added vertical dividers, restricted links to authenticated users |
| gateway/sds_gateway/templates/pages/home.html | Added branding display with institution names and optional brand image banner |
| gateway/sds_gateway/templates/pages/about.html | Added optional brand image background to description section |
| gateway/sds_gateway/templates/users/dataset_list.html | Changed title from "My Datasets" to "Datasets" for consistency |
| gateway/sds_gateway/templates/users/partials/my_datasets_tab.html | Updated comments for consistency |
| gateway/sds_gateway/templates/users/file_list.html | Restructured layout, added date auto-fill logic, improved search UI |
| gateway/sds_gateway/static/js/components.js | Implemented AbortController pattern for request cancellation in SearchManager |
| gateway/sds_gateway/static/js/file-list.js | Updated to use AbortController signal, added minimum loading time, improved button state management |
| gateway/sds_gateway/static/css/variables.css | Added homepage color variable for institution highlighting |
| gateway/sds_gateway/static/css/spectrumx_theme.css | Added styles for brand images, navbar branding, and institution highlighting |
| gateway/docs/changelog.md | Added comprehensive deployment instructions for operators |
| .pre-commit-config.yaml | Increased max file size check from default to 1MB |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div class="brand-banner mb-4" | ||
| style="--brand-image: url('{{ SDS_BRAND_IMAGE_URL }}')"></div> |
There was a problem hiding this comment.
The SDS_BRAND_IMAGE_URL is inserted directly into inline CSS without proper sanitization. While the _get_brand_image_url function in settings/base.py validates the format, if an attacker can control the SDS_BRAND_IMAGE_PATH environment variable, they could inject malicious CSS. Django's template auto-escaping does not protect against CSS injection in style attributes. Consider using a more restrictive validation (e.g., allowlist of domains for external URLs) or using the Content-Security-Policy header to restrict image sources.
| <div class="brand-banner mb-4" | |
| style="--brand-image: url('{{ SDS_BRAND_IMAGE_URL }}')"></div> | |
| <div class="brand-banner mb-4"> | |
| <img src="{{ SDS_BRAND_IMAGE_URL }}" alt="Brand banner" class="img-fluid" /> | |
| </div> |
| role="navigation" | ||
| aria-label="Main navigation"> | ||
| aria-label="Main navigation" | ||
| {% if SDS_BRAND_IMAGE_URL %}style="--navbar-bg-image: url('{{ SDS_BRAND_IMAGE_URL }}')"{% endif %}> |
There was a problem hiding this comment.
The SDS_BRAND_IMAGE_URL is inserted directly into inline CSS without proper sanitization. While the _get_brand_image_url function in settings/base.py validates the format, if an attacker can control the SDS_BRAND_IMAGE_PATH environment variable, they could inject malicious CSS. Django's template auto-escaping does not protect against CSS injection in style attributes. Consider using a more restrictive validation (e.g., allowlist of domains for external URLs) or using the Content-Security-Policy header to restrict image sources.
| {% if SDS_BRAND_IMAGE_URL %}style="--navbar-bg-image: url('{{ SDS_BRAND_IMAGE_URL }}')"{% endif %}> | |
| {% if SDS_BRAND_IMAGE_URL %}style="--navbar-bg-image: url('{{ SDS_BRAND_IMAGE_URL|urlencode }}')"{% endif %}> |
| <div class="p-5 mb-4 bg-light rounded-3"> | ||
| <div id="descriptionSection" | ||
| class="p-5 mb-4 bg-light rounded-3 description-hero" | ||
| {% if SDS_BRAND_IMAGE_URL %}style="--description-bg-image: url('{{ SDS_BRAND_IMAGE_URL }}')"{% endif %}> |
There was a problem hiding this comment.
The SDS_BRAND_IMAGE_URL is inserted directly into inline CSS without proper sanitization. While the _get_brand_image_url function in settings/base.py validates the format, if an attacker can control the SDS_BRAND_IMAGE_PATH environment variable, they could inject malicious CSS. Django's template auto-escaping does not protect against CSS injection in style attributes. Consider using a more restrictive validation (e.g., allowlist of domains for external URLs) or using the Content-Security-Policy header to restrict image sources.
| {% if SDS_BRAND_IMAGE_URL %}style="--description-bg-image: url('{{ SDS_BRAND_IMAGE_URL }}')"{% endif %}> | |
| {% if SDS_BRAND_IMAGE_URL %}style="--description-bg-image: url('{{ SDS_BRAND_IMAGE_URL|urlencode }}')"{% endif %}> |
| // Only set end date if it's not already set or if it's not greater than start date | ||
| if (!currentEndDate) { | ||
| startDate.setDate(startDate.getDate() + 1); | ||
| const nextDay = startDate.toISOString().split('T')[0]; | ||
| endDateInput.value = nextDay; | ||
| } else { | ||
| const endDate = new Date(currentEndDate); | ||
| if (endDate <= startDate) { |
There was a problem hiding this comment.
The date comparison logic has a potential timezone issue. When creating Date objects from date strings (YYYY-MM-DD), JavaScript interprets them as UTC midnight. However, the comparison "endDate <= startDate" at line 480 should be "endDate < startDate" because if both dates are equal (same day), there's no need to auto-correct the end date to the next day. With the current logic, if a user selects the same start and end date intentionally, the system will incorrectly force the end date to be the next day.
| // Only set end date if it's not already set or if it's not greater than start date | |
| if (!currentEndDate) { | |
| startDate.setDate(startDate.getDate() + 1); | |
| const nextDay = startDate.toISOString().split('T')[0]; | |
| endDateInput.value = nextDay; | |
| } else { | |
| const endDate = new Date(currentEndDate); | |
| if (endDate <= startDate) { | |
| // Only set end date if it's not already set or if it's earlier than start date | |
| if (!currentEndDate) { | |
| startDate.setDate(startDate.getDate() + 1); | |
| const nextDay = startDate.toISOString().split('T')[0]; | |
| endDateInput.value = nextDay; | |
| } else { | |
| const endDate = new Date(currentEndDate); | |
| if (endDate < startDate) { |
| assert "SDS_BRANDED_SITE_NAME" in context | ||
| assert "SDS_FULL_INSTITUTION_NAME" in context | ||
| assert "SDS_SHORT_INSTITUTION_NAME" in context | ||
| assert "SDS_PROGRAMMATIC_SITE_NAME" in context | ||
| assert "SDS_SITE_FQDN" in context |
There was a problem hiding this comment.
The test for the branding context processor is missing a test for SDS_BRAND_IMAGE_URL. The branding function returns SDS_BRAND_IMAGE_URL at line 46 in context_processors.py, but the test only checks for the other five branding settings. This creates incomplete test coverage for the branding context processor.
| def _get_brand_image_url() -> str | None: | ||
| """Resolve brand image path to a usable URL. | ||
|
|
||
| Supports: | ||
| - Complete URLs (http://, https://) | ||
| - Local static paths (converted to /static/...) | ||
| - Empty strings (returns None) | ||
| """ | ||
| image_path: str = env.str("SDS_BRAND_IMAGE_PATH", default="") | ||
|
|
||
| if not image_path: | ||
| return None | ||
|
|
||
| # If it's already a complete URL, use as-is | ||
| if image_path.startswith(("http://", "https://")): | ||
| return image_path | ||
|
|
||
| # If it's a relative path, prepend /static/ | ||
| if not image_path.startswith("/"): | ||
| image_path = f"/{image_path}" | ||
|
|
||
| if not image_path.startswith("/static/"): | ||
| image_path = f"/static{image_path}" | ||
|
|
||
| return image_path |
There was a problem hiding this comment.
The _get_brand_image_url function in base.py lacks test coverage. This function has multiple code paths (empty string, http/https URLs, relative paths, paths with/without /static/) that should be tested to ensure the URL resolution logic works correctly in all cases. Without tests, future changes could break this functionality without being detected.
Uh oh!
There was an error while loading. Please reload this page.