Feature kpoland dataset new version #227
Conversation
klpoland
added
feature
|
In this PR:
|
klpoland
force-pushed
the
feature-kpoland-dataset-new-version
branch
from
f639183 to
6af27e8
Compare
|
For Monday: Added versioning AND consolidated some javascript functionality (which is why there's so many deletions). Refactoring/updating tests in Jest and Python to match and pushing that shortly.... |
gateway/sds_gateway/api_methods/serializers/dataset_serializers.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
just checking if these "keyword" removals are intentional
There was a problem hiding this comment.
no.... I missed where it was being used bc it's in an unexpected place. I'm going to move the KeywordAutocomplete class into a JS file.
klpoland
force-pushed
the
feature-kpoland-dataset-new-version
branch
from
08078cb to
0b79b4c
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds dataset versioning functionality to the SDS Gateway, allowing users to create new versions of datasets that inherit the files and captures from the previous version. The PR also includes significant refactoring of the frontend JavaScript architecture and templates.
Changes:
- Added dataset versioning with
version(IntegerField) andprevious_version(ForeignKey) fields to the Dataset model - Created
DatasetVersioningViewwith transaction-based locking to prevent race conditions during version creation - Refactored frontend code by extracting
KeywordAutocompleteandDownloadInstructionsManagerinto separate, testable modules - Standardized modal IDs from
share-modal-*toshareModal-*throughout the codebase - Refactored
ListRefreshManagerfrom singleton to class-based instantiation pattern - Added comprehensive test coverage for new and refactored JavaScript components
- Simplified access control logic by removing M2M relationship support (FK-only)
Reviewed changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| gateway/sds_gateway/users/views.py | Added DatasetVersioningView with transaction locking for version creation; added AJAX support for dataset list view |
| gateway/sds_gateway/users/urls.py | Added dataset-versioning endpoint; reorganized URL patterns; removed publish_dataset URL (bug) |
| gateway/sds_gateway/api_methods/models.py | Added version (IntegerField) and previous_version (ForeignKey) fields; added user_can_advance_version permission check |
| gateway/sds_gateway/api_methods/migrations/0020_*.py | Migration to add versioning fields with data conversion for existing version strings |
| gateway/sds_gateway/api_methods/serializers/dataset_serializers.py | Added can_advance_version field to dataset serializer |
| gateway/sds_gateway/api_methods/utils/asset_access_control.py | Removed M2M relationship support, simplified to FK-only access control (potential breaking change) |
| gateway/sds_gateway/templates/users/partials/dataset_version_control.html | New modal template for dataset version creation confirmation |
| gateway/sds_gateway/templates/users/components/dataset_list_table.html | New component for AJAX-updatable dataset list table |
| gateway/sds_gateway/static/js/actions/VersioningActionManager.js | New manager for handling dataset version creation |
| gateway/sds_gateway/static/js/actions/DownloadInstructionsManager.js | Extracted download instructions copy functionality into separate manager |
| gateway/sds_gateway/static/js/search/KeywordChipInput.js | Extracted KeywordAutocomplete class from inline script |
| gateway/sds_gateway/static/js/core/DOMUtils.js | Added modal management methods (openModal, closeModal, showModalLoading, etc.) |
| gateway/sds_gateway/static/js/core/APIClient.js | Refactored ListRefreshManager from singleton to class-based pattern |
| gateway/sds_gateway/static/js/core/PageLifecycleManager.js | Added modal pre-initialization and versioning manager setup |
| Multiple test files | Added comprehensive Jest tests for new and refactored components |
| Multiple template files | Standardized modal IDs and removed inline JavaScript |
Comments suppressed due to low confidence (1)
gateway/sds_gateway/users/urls.py:30
- The import for
user_publish_dataset_viewhas been removed from the imports, but this view is still being used by the PublishActionManager.js file at line 248 (/users/publish-dataset/${datasetUuid}/). The URL pattern was also removed from urlpatterns. This will break the publish dataset functionality.
The removed URL pattern was:
path(
"publish-dataset/<uuid:dataset_uuid>/",
user_publish_dataset_view,
name="publish_dataset",
),This needs to be restored to maintain the publish functionality.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,33 @@ | |||
| # Generated by Django 4.2.26 on 2025-11-25 20:50 | |||
There was a problem hiding this comment.
The migration timestamp indicates a future date: "2025-11-25 20:50". This appears to be incorrect as the current date context indicates it's February 2, 2026. Migration timestamps should reflect when they were actually generated. This should be updated to the correct date when the migration was created.
| # Generated by Django 4.2.26 on 2025-11-25 20:50 | |
| # Generated by Django 4.2.26 |
| try: | ||
| new_dataset = Dataset.objects.create(**dataset_data) | ||
| except IntegrityError: | ||
| # If we get an IntegrityError, another request may have created it | ||
| # Fetch the existing version and return it | ||
| existing_version = Dataset.objects.filter( | ||
| previous_version=locked_dataset, | ||
| version=new_version, | ||
| owner=request_user, | ||
| is_deleted=False, | ||
| ).first() | ||
| if existing_version: | ||
| return existing_version | ||
| # Re-raise if we can't find it (unexpected error) | ||
| raise |
There was a problem hiding this comment.
In the version creation logic, the code catches IntegrityError to handle race conditions. However, IntegrityError can occur for reasons other than the specific race condition being guarded against (e.g., constraint violations on other fields).
The catch block queries for an existing version with the same previous_version, version, and owner. However, if the IntegrityError was caused by a different constraint violation (not the race condition), this query might return None, and the exception will be re-raised without a clear error message about what went wrong.
Consider adding more specific error handling or logging to distinguish between expected race condition IntegrityErrors and unexpected ones. For example:
except IntegrityError as e:
# Log the error for debugging
log.warning(f"IntegrityError during version creation: {e}")
# Try to find existing version
existing_version = Dataset.objects.filter(...).first()
if existing_version:
return existing_version
# Re-raise with more context
raise IntegrityError(f"Failed to create dataset version: {e}") from e| no_copy_fields = [ | ||
| "uuid", | ||
| "created_at", | ||
| "updated_at", | ||
| "status", | ||
| "is_public", | ||
| "shared_with", | ||
| ] | ||
|
|
||
| dataset_data = { | ||
| field.name: getattr(locked_dataset, field.name) | ||
| for field in locked_dataset._meta.get_fields() # noqa: SLF001 | ||
| if hasattr(field, "name") | ||
| and field.name not in no_copy_fields | ||
| and not field.many_to_many | ||
| and not field.one_to_many | ||
| and not field.one_to_one | ||
| } | ||
| dataset_data["owner"] = request_user | ||
| dataset_data["version"] = new_version | ||
| dataset_data["previous_version"] = locked_dataset |
There was a problem hiding this comment.
The dataset field copying logic iterates over all fields and excludes M2M, one-to-many, and one-to-one relationships. However, it doesn't explicitly handle the new previous_version field that was just added.
While previous_version is explicitly set on line 3567, if the iteration includes it in the field list before that assignment, it could cause issues. Consider adding "previous_version" to the no_copy_fields list to make the exclusion explicit and avoid any potential confusion or bugs:
no_copy_fields = [
"uuid",
"created_at",
"updated_at",
"status",
"is_public",
"shared_with",
"previous_version", # Add this
]This makes the code's intent clearer and more maintainable.
| window.APIError = APIError; | ||
| window.LoadingStateManager = LoadingStateManager; | ||
| window.ListRefreshManager = new ListRefreshManager(); | ||
| window.ListRefreshManager = ListRefreshManager; |
There was a problem hiding this comment.
The ListRefreshManager has been changed from a singleton instance to a class that needs to be instantiated. However, several files still reference it as if it were a singleton (e.g., window.ListRefreshManager.refreshList() or similar).
In ShareActionManager.js line 251-257, the code checks for window.listRefreshManager (lowercase 'l') and calls loadTable(), but in dataset_list.html line 114-118, a new instance is created with window.listRefreshManager = new ListRefreshManager(...).
This pattern is correct, but you need to ensure:
- All references use the correct casing (
listRefreshManagernotListRefreshManager) - The instance is created before it's used
- The
loadTablemethod signature matches what's being called
The change from a singleton to an instance pattern is good for flexibility, but make sure all calling code has been updated appropriately.
| window.ListRefreshManager = ListRefreshManager; | |
| // NOTE: ListRefreshManager is a class and should be instantiated explicitly (e.g., window.listRefreshManager = new ListRefreshManager(...)). |
No description provided.