Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: use usage_key instead of block_id as location identifier for scorm data #71

Merged
merged 4 commits into from
Apr 23, 2024
Merged

fix: use usage_key instead of block_id as location identifier for scorm data #71

merged 4 commits into from
Apr 23, 2024

Conversation

Danyal-Faheem
Copy link
Contributor

@Danyal-Faheem Danyal-Faheem commented Apr 2, 2024 •
edited
Loading

Fixes #70.

Changes

  • Added a new base_path for the scorm folder. The previous one was using the block_id as an identifier for the directory name but when exporting a course and importing it, the block_id stayed the same causing both the courses to share the scorm directory. Now, the base_path is a sha1 hash of the usage_key which is unique across different courses as it also takes into account the course_key as well.
  • The previous base_path is still kept for backwards compatibility and that path is returned in case storage exists on that path, meaning that the scorm module was created before this change.

Caveats

  • If a scorm unit has already been initialized, i.e, scorm file has been uploaded, then duplicating that subsection or exporting and importing that course will still break the scorm module as earlier due to the path not being updated to the new version.
  • Due to above point, this change will only effect the scorm modules initialized after this PR has been merged. Any course already created will still be facing the issue mentioned in Importing a course with scorm and changing the scorm file deletes the exported course scorm file as well #70 until the course author reupload scorm files and save again, i.e studio_submit() is called again.

@Danyal-Faheem Danyal-Faheem force-pushed the danyalfaheem/fix-course-import-duplicate-path branch from 332719d to 9fb3da0 Compare April 22, 2024 08:40
ziafazal
ziafazal reviewed Apr 22, 2024
View reviewed changes
Copy link
Collaborator

@ziafazal ziafazal left a comment

Choose a reason for hiding this comment

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

@Danyal-Faheem I think we should avoid uploading of content file it does not make sense to extract old package when course is imported. I think in case of course import scorm package should become unavailable in new course whereas in previous course it should work fine.

openedxscorm/scormxblock.py Outdated
@@ -300,6 +319,17 @@ def studio_submit(self, request, _suffix):
package_file = request.params["file"].file
self.update_package_meta(package_file)

# First, save scorm file in the storage for later use
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not getting the usage of this code. Why we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was for uploading and saving the content file at the storage backend. However, if we don't need to save the file this isn't required anymore.

@Danyal-Faheem
Copy link
Contributor Author

@Danyal-Faheem I think we should avoid uploading of content file it does not make sense to extract old package when course is imported. I think in case of course import scorm package should become unavailable in new course whereas in previous course it should work fine.

@ziafazal okay, I will remove the code for saving the content file and just fix the path then.

@Danyal-Faheem
fix: use usage_key instead of block_id as location identifier
557d132 
This is because block_id is maintained across course export/import
Both the courses try to access the same data on storage
Manipulating data in one course will overwrite the data for the other one, breaking it
@Danyal-Faheem Danyal-Faheem changed the title fix: extract package_file again if storage not found in case of course import or unit duplicate fix: use usage_key instead of block_id as location identifier for scorm data Apr 23, 2024
@Danyal-Faheem Danyal-Faheem force-pushed the danyalfaheem/fix-course-import-duplicate-path branch from 9fb3da0 to 557d132 Compare April 23, 2024 06:07
Danyal-Faheem and others added 3 commits April 23, 2024 11:08
@Danyal-Faheem
fix: use usage_key instead of block_id as location identifier
89d57a6 
This is because block_id is maintained across course export/import
Both the courses try to access the same data on storage
Manipulating data in one course will overwrite the data for the other one, breaking it
@Danyal-Faheem
Merge branch 'overhangio:master' into danyalfaheem/fix-course-import-…
44fa18f 
…duplicate-path
@Danyal-Faheem
docs: add changelog entry
b19fb92
ziafazal
ziafazal approved these changes Apr 23, 2024
View reviewed changes
Copy link
Collaborator

@ziafazal ziafazal left a comment

Choose a reason for hiding this comment

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

👍

@ziafazal ziafazal merged commit 6401bba into overhangio:master Apr 23, 2024
ziafazal pushed a commit that referenced this pull request May 27, 2024
@Danyal-Faheem
fix: use usage_key instead of block_id as location identifier for sco…
4233481 
…rm data (#71)

* fix: use usage_key instead of block_id as location identifier
This is because block_id is maintained across course export/import
Both the courses try to access the same data on storage
Manipulating data in one course will overwrite the data for the other one, breaking it
@igobranco igobranco mentioned this pull request Jun 20, 2024
Closed
@Danyal-Faheem Danyal-Faheem deleted the danyalfaheem/fix-course-import-duplicate-path branch August 28, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ziafazal ziafazal ziafazal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Tutor project management
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Importing a course with scorm and changing the scorm file deletes the exported course scorm file as well
2 participants
@Danyal-Faheem @ziafazal