-
Notifications
You must be signed in to change notification settings - Fork 62
SCORM contentstore sync for OLX export/import #113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| - [Feature] Optionally mirror the SCORM zip into the course contentstore on upload and rehydrate the unpacked tree from the contentstore on first access. This makes blocks survive OLX course export/import without breaking the extraction path keyed by `sha1(usage_key)` introduced in PR #71. Disabled by default; enable with `XBLOCK_SETTINGS["ScormXBlock"]["CONTENTSTORE_SYNC_ENABLED"] = True`. (by @Syed-Ali-Abbas-568) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: The changelog entry can be reworded to better describe the change. We don't need to link the PR either. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import io | ||
| import json | ||
| import hashlib | ||
| import os | ||
|
|
@@ -230,8 +231,12 @@ def assets_proxy(self, request, suffix): | |
| ------- | ||
| Response object containing the content of the requested file with the appropriate content type. | ||
| """ | ||
| # If this block was just imported via OLX, the unpacked files may be | ||
| # missing from default_storage. Rehydrate from the contentstore before | ||
| # serving. No-op when the feature is disabled or the tree exists. | ||
| self._rehydrate_from_contentstore() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assets_proxy is called on every request to the LMS and page load. We don't need to extract scorm files every time. Unless, there is a specific reason for this, we should only extract once when the xblock is initialized or on course import. |
||
| file_name = os.path.basename(suffix) | ||
| file_path = self.find_file_path(file_name) | ||
| file_path = self.find_file_path(file_name) | ||
| file_type, _ = mimetypes.guess_type(file_name) | ||
| with self.storage.open(file_path) as response: | ||
| file_content = response.read() | ||
|
|
@@ -299,6 +304,10 @@ def studio_submit(self, request, _suffix): | |
| try: | ||
| self.extract_package(package_file) | ||
| self.update_package_fields() | ||
| # Mirror the original zip into the course's contentstore so that | ||
| # OLX export bundles it and OLX import re-creates it under the new | ||
| # course_key. Best-effort and feature-flagged off by default. | ||
| self._save_zip_to_contentstore(package_file) | ||
| except ScormError as e: | ||
| response["errors"].append(e.args[0]) | ||
|
|
||
|
|
@@ -384,7 +393,12 @@ def extract_package(self, package_file): | |
| def index_page_url(self): | ||
| if not self.package_meta or not self.index_page_path: | ||
| return "" | ||
|
|
||
|
|
||
| # If this block was just imported via OLX, the unpacked files may be | ||
| # missing from default_storage. Rehydrate from the contentstore before | ||
| # building the URL. No-op when the feature is disabled or the tree exists. | ||
| self._rehydrate_from_contentstore() | ||
|
|
||
| # Serve assets by proxying them through the LMS by default | ||
| if self.xblock_settings.get("PROXY_ASSETS_LMS", True): | ||
| return f"{self.proxy_base_url}/{self.index_page_path}" | ||
|
|
@@ -884,6 +898,139 @@ def workbench_scenarios(): | |
| ), | ||
| ] | ||
|
|
||
| @property | ||
| def contentstore_sync_enabled(self): | ||
| """ | ||
| Whether to mirror the SCORM zip to the course's contentstore on save and | ||
| rehydrate it from the contentstore on read. | ||
|
|
||
| Disabled by default; opt in by setting | ||
| XBLOCK_SETTINGS["ScormXBlock"]["CONTENTSTORE_SYNC_ENABLED"] = True. | ||
| """ | ||
| return bool(self.xblock_settings.get("CONTENTSTORE_SYNC_ENABLED", False)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we type casting this? We've provided a default value here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am still not sure whether this should be False by default or not considering it is a request feature. @ahmed-arb What do you think? |
||
|
|
||
| def _contentstore_asset_path(self): | ||
| """ | ||
| Filename used when storing the SCORM zip in the course's contentstore. | ||
|
|
||
| The path is keyed by the package sha1 so that OLX export/import carries | ||
| the same asset name into the new course. | ||
| """ | ||
| sha1 = self.package_meta.get("sha1") if self.package_meta else None | ||
| if not sha1: | ||
| return None | ||
| return f"scorm_packages/{sha1}.zip" | ||
|
|
||
| def _save_zip_to_contentstore(self, package_file): | ||
| """ | ||
| Mirror the original SCORM zip into the course's contentstore. | ||
|
|
||
| Best-effort: any failure (missing imports, no course context, contentstore | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Lets try not to use obvious AI generated comments |
||
| error) is logged and swallowed so that the upload still succeeds. | ||
| """ | ||
| if not self.contentstore_sync_enabled: | ||
| return | ||
| try: | ||
| from xmodule.contentstore.django import contentstore | ||
| from xmodule.contentstore.content import StaticContent | ||
|
Comment on lines
+934
to
+935
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless these packages have been deprecated or renamed somehow, I believe xmodule is available in the openedx-platform itself and we don't need to put a try catch here as we can assume openedx is installed. Do correct me if I'm wrong. My knowledge on this might be a little stale. |
||
| except ImportError: | ||
| logger.warning( | ||
| "xmodule.contentstore unavailable; skipping SCORM zip mirror" | ||
| ) | ||
| return | ||
|
|
||
| course_key = getattr(self.runtime, "course_id", None) | ||
| asset_path = self._contentstore_asset_path() | ||
| if course_key is None or asset_path is None: | ||
| return | ||
|
|
||
| try: | ||
| asset_key = StaticContent.compute_location(course_key, asset_path) | ||
| package_file.seek(0) | ||
| data = package_file.read() | ||
| package_file.seek(0) | ||
| content = StaticContent( | ||
| asset_key, | ||
| asset_path, | ||
| "application/zip", | ||
| data, | ||
| locked=True, | ||
| ) | ||
| contentstore().save(content) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be deleting the previous scorm file when uploading the new one. This replicates the behaviour already defined here |
||
| logger.info("Mirrored SCORM zip to contentstore at %s", asset_key) | ||
| except Exception: | ||
| logger.exception("Failed to mirror SCORM zip to contentstore") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should log the actual error that we face for debugging purposes. |
||
|
|
||
| def _fetch_zip_from_contentstore(self): | ||
| """ | ||
| Fetch the original SCORM zip bytes from the course's contentstore. | ||
|
|
||
| Returns the bytes payload, or None when the feature is disabled, the | ||
| contentstore is unavailable, or no asset is found for this block. | ||
| """ | ||
| if not self.contentstore_sync_enabled: | ||
| return None | ||
| try: | ||
| from xmodule.contentstore.django import contentstore | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same issue as above. |
||
| from xmodule.contentstore.content import StaticContent | ||
| except ImportError: | ||
| return None | ||
| try: | ||
| from xmodule.exceptions import NotFoundError | ||
| except ImportError: | ||
| NotFoundError = None # type: ignore | ||
|
|
||
| course_key = getattr(self.runtime, "course_id", None) | ||
| asset_path = self._contentstore_asset_path() | ||
| if course_key is None or asset_path is None: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should log this issue here rather than just exiting. |
||
| return None | ||
|
|
||
| try: | ||
| asset_key = StaticContent.compute_location(course_key, asset_path) | ||
| content = contentstore().find(asset_key) | ||
| return content.data | ||
| except Exception as exc: | ||
| if NotFoundError is not None and isinstance(exc, NotFoundError): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line seems very confusing. Maybe it can be made more readable? |
||
| return None | ||
| logger.exception("Failed to fetch SCORM zip from contentstore") | ||
| return None | ||
|
|
||
| def _is_extracted_tree_missing(self): | ||
| """ | ||
| Return True when package_meta is set but the extracted tree is empty. | ||
|
|
||
| After OLX import the metadata is restored but the unpacked files are not, | ||
| so this is the signal to rehydrate from the contentstore. | ||
| """ | ||
| if not self.package_meta: | ||
| return False | ||
| return not self.path_exists(self.extract_folder_path) | ||
|
|
||
| def _rehydrate_from_contentstore(self): | ||
| """ | ||
| Re-extract the SCORM zip from the contentstore back into default_storage. | ||
|
|
||
| No-op unless the feature is enabled, package_meta is present, and the | ||
| extracted tree is missing. Returns True on a successful rehydration. | ||
| """ | ||
| if not self.contentstore_sync_enabled: | ||
| return False | ||
| if not self._is_extracted_tree_missing(): | ||
| return False | ||
| data = self._fetch_zip_from_contentstore() | ||
| if not data: | ||
| return False | ||
| try: | ||
| self.extract_package(io.BytesIO(data)) | ||
| logger.info( | ||
| "Rehydrated SCORM package from contentstore for usage_id=%s", | ||
| self.scope_ids.usage_id, | ||
| ) | ||
| return True | ||
| except Exception: | ||
| logger.exception("Failed to rehydrate SCORM package from contentstore") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same issue as above. |
||
| return False | ||
|
|
||
| @property | ||
| def storage(self): | ||
| """ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for such details. We can just mention that this is off by default for xyz reason.