-
Notifications
You must be signed in to change notification settings - Fork 167
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
Extract metadata for ims scorm cp #4258
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
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.xml
is present in file or notimsmetadata.xml
we need to check this file as wellextractIMSMetadata
Manual verification steps performed
.zip
format:imsmanifest.xml
andimsmetadata.xml
file 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:
CHANGELOG
label been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocs
label has been added if this introduces a change that needs to be updated in the user docs?requirements.txt
files also included in this PRStudio-specifc:
notranslate
class been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages
,components
, andlayouts
directories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarn
andpip
)