-
Notifications
You must be signed in to change notification settings - Fork 262
Extract metadata for ims scorm cp #4258
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
base: unstable
Are you sure you want to change the base?
Extract metadata for ims scorm cp #4258
Conversation
- Create topic and resouce node for the metadata
cb939ec to
33b1680
Compare
33b1680 to
90102de
Compare
- adding test cases for extractIMSMetadata
rtibbles
left a comment
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.
Some minor tweaks needed, but this is looking very good!
Note: we will hold off merge until unstable has been released, so that we can release the H5P metadata extraction sooner, then this will be merged and released in a later release!
| @input="trackSelect" | ||
| @removed="handleRemoved" | ||
| /> | ||
| <div v-if="getChildren !== undefined"> |
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.
Should change this to a length check on the array!
| }); | ||
| } else if (file.metadata.folders) { | ||
| this.createNode('topic', file.metadata).then(newNodeId => { | ||
| file.metadata.folders.forEach(org => { |
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.
Update org here to folder.
| this.createNode('topic', file.metadata).then(newNodeId => { | ||
| file.metadata.folders.forEach(org => { | ||
| this.createNode('topic', org, newNodeId).then(topicNodeId => { | ||
| org.files.forEach(orgFile => { |
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.
orgFile to folderFile
| return File.uploadUrl({ | ||
| checksum: file.checksum, | ||
| size: file.file_size, | ||
| type: 'application/zip', |
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.
Let's double check if we need to specify this explicitly, or if it should be inferred by existing functionality.
| total: file.size, | ||
| }; | ||
| if (index === 0) { | ||
| this.selected = [resourceNodeId]; |
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.
Let's change this to set this.selected if we haven't already set this.selected to something - so only the first finalized node gets selected.
| }); | ||
| }); | ||
| it('extractIMSMetadata should extract metadata from imsmanifest.xml', async () => { | ||
| // const manifestFile = get_imsmanifest_file({ title: 'Test file' }); |
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.
Clean up comments!
| <imsmd:lom> | ||
| <imsmd:general> | ||
| <imsmd:title> | ||
| <imsmd:langstring xml:lang="en">Test File</imsmd:langstring> |
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.
lang should be und here.
| ) { | ||
| metadata.language = xmlDoc | ||
| .getElementsByTagName('lomes:idiom')[0] | ||
| .children[0].textContent.replace(/ {2}|\r\n|\n|\r/gm, ''); |
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.
Just double check whether trim will do the same job here!
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.
The assertions in the tests should validate that this is working properly!
| .getElementsByTagName('lomes:idiom')[0] | ||
| .textContent.replace(/ {2}|\r\n|\n|\r/gm, '') !== 'und' | ||
| ) { | ||
| metadata.language = xmlDoc |
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.
For language extraction let's replicate the validation we are doing in H5P to check this is a supported language code.
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.
Can also add some more tests for unhappy paths!
| const IMS_PRESETS = [ | ||
| FormatPresetsNames.QTI, | ||
| FormatPresetsNames.HTML5_DEPENDENCY, | ||
| FormatPresetsNames.HTML5_ZIP, |
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.
Note for me - I may need to consider an IMSCP preset format.
2db6a02 to
6c94668
Compare
| xmlDoc | ||
| .getElementsByTagName('lomes:idiom')[0] | ||
| .textContent.replace(/ {2}|\r\n|\n|\r/gm, '') !== 'und' | ||
| LanguagesMap.has(xmlDoc.getElementsByTagName('lomes:idiom')[0].textContent.trim()) && |
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.
Could avoid a bit of repetition here and define outside of the if statement:
const language = xmlDoc.getElementsByTagName('lomes:idiom').length ? xmlDoc.getElementsByTagName('lomes:idiom')[0].textContent.trim() : 'und';
(defaulting to the disallowed und)
then you can do the checks and assignment against this value instead.
This code aims to extract metadata from IMS content package.
This Pr is a part of GSoC Project linked with the issue #4081.
Summary
Description of the change(s) you made
imsmanifest.xmlis present in file or notimsmetadata.xmlwe need to check this file as wellextractIMSMetadataManual verification steps performed
.zipformat:imsmanifest.xmlandimsmetadata.xmlfile and read the file as text.Comments
Checking for sub manifest files present in content packe is yet to be implemented.
Contributor's Checklist
PR process:
CHANGELOGlabel been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocslabel has been added if this introduces a change that needs to be updated in the user docs?requirements.txtfiles also included in this PRStudio-specifc:
notranslateclass been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages,components, andlayoutsdirectories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarnandpip)