Skip to content

feat: add SCORM support to uploadcontent and uploadlargecontent plugins#783

Open
romitshah02 wants to merge 6 commits into
Sunbird-Knowlg:masterfrom
romitshah02:master
Open

feat: add SCORM support to uploadcontent and uploadlargecontent plugins#783
romitshah02 wants to merge 6 commits into
Sunbird-Knowlg:masterfrom
romitshah02:master

Conversation

@romitshah02
Copy link
Copy Markdown

Summary

This pull request adds support for uploading SCORM-compliant content packages to both the org.ekstep.uploadcontent and org.ekstep.uploadlargecontent plugins.

Changes included:

  • SCORM Detection: Added logic to inspect uploaded zip files for imsmanifest.xml using JSZip to identify SCORM packages.
  • MIME Type Handling: SCORM packages are now correctly assigned the application/vnd.ekstep.scorm-archive mime type during upload.
  • File Validation: Non-SCORM zip files are rejected in the large content upload plugin; only SCORM-compliant zips are accepted.

Files Modified:

  • org.ekstep.uploadcontent-1.5/editor/uploadapp.js
  • org.ekstep.uploadcontent-1.5/editor/upload.html
  • org.ekstep.uploadlargecontent-1.0/editor/uploadlargecontentapp.js
  • org.ekstep.uploadlargecontent-1.0/editor/uploadlargecontent.html

Copy link
Copy Markdown
Collaborator

@sntiwari1 sntiwari1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: SCORM Support for Upload Plugins

Thank you for adding SCORM support to both upload plugins. The feature works conceptually but there are several bugs and quality issues that should be addressed before merging.

Critical Issues

1. Stale $scope.isScorm state in uploadapp.js
In the onSubmit callback, when a zip is uploaded but does NOT contain imsmanifest.xml, $scope.isScorm is never explicitly set to false. This means: if a user first uploads a SCORM zip (sets isScorm=true), then resets and uploads a regular HTML zip, the HTML zip will be incorrectly assigned the application/vnd.ekstep.scorm-archive mime type.

Fix: add an else branch to reset the flag:

JSZip.loadAsync(e.target.result).then(function(zip) {
    $scope.isScorm = zip.file("imsmanifest.xml") !== null; // always set, don't leave stale
    $scope.upload();
});

2. Missing .catch() handler on JSZip promise (both plugins)
If the zip file is corrupted or unreadable, JSZip.loadAsync will reject silently. In uploadapp.js, this leaves the UI frozen (actions hidden, no loader shown). In uploadlargecontentapp.js, the same issue applies.

Fix: always add a rejection handler:

JSZip.loadAsync(e.target.result).then(function(zip) {
    // ...
}).catch(function(err) {
    $scope.toasterMsgHandler('error', 'Unable to read zip file. Please try again.');
    $scope.uploader.reset();
    $scope.showLoader(false);
    // re-show upload actions
    $('#qq-upload-actions').show();
});

Medium Issues

3. Angular digest cycle not triggered for $scope.isScorm in uploadapp.js
The JSZip.loadAsync callback runs outside Angular's digest cycle. Setting $scope.isScorm = true inside the .then() won't trigger a digest in uploadapp.js (unlike uploadlargecontentapp.js which eventually calls $safeApply() via showLoader). The $scope.upload() call relies on $scope.isScorm being set correctly before Angular evaluates any bindings.

Recommend wrapping the callback body in $scope.$apply() or using $scope.$safeApply() to ensure consistency.

4. Outdated and duplicated JSZip library

  • The bundled jszip.min.js is version 3.1.5 from 2016 — the current release is 3.10.x. Older versions may have unpatched vulnerabilities.
  • The exact same file is duplicated in both plugins (org.ekstep.uploadcontent-1.5/editor/libs/ and org.ekstep.uploadlargecontent-1.0/editor/libs/).

Recommend using a CDN reference or a shared libs/ folder at the root level, and upgrade to the latest stable version.

Low / Nit

5. Error message in uploadlargecontentapp.js is misleading
The error shown when a non-SCORM zip is uploaded says:

"Invalid content type (supported type: mp4, webm and scorm)"

The word "scorm" will be confusing to users since they upload a .zip file — they don't know it's a SCORM package until after it's validated. Suggest:

"Invalid content type. Supported types: mp4, webm, or a SCORM-compliant zip (containing imsmanifest.xml)"

6. detectMimeType still returns html-archive for zip in both plugins
The zip case in detectMimeType returns application/vnd.ekstep.html-archive, and SCORM is then applied as a post-hoc override via the $scope.isScorm flag. This tight coupling is fragile — a future refactor could drop the override and break SCORM uploads silently. Consider either passing SCORM context into detectMimeType or renaming the override logic to be more explicit.

7. Dead variable in uploadlargecontentapp.js generatePreSignedUrl

const file = $scope.selectedFile   // assigned but never used
const url = $scope.submitUri        // assigned but never used  

These were likely left over from debugging.

@romitshah02 romitshah02 requested a review from sntiwari1 May 14, 2026 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants