Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      Here's a scorm package that can be used:
      http://moodle.org/mod/data/view.php?d=50&rid=1655

      This feature adds the ability to drag and drop scorm packages into a course
      Log in as an admin
      Enter a course and turn on editing
      drag a SCORM zip file over a section and choose to add as a SCORM package
      After SCORM has been added to the course, enter SCORM and make sure it loads correctly

      Please note there are a couple of known issues, please see the linked issues on MDL-22504
      Also note that AICC Packages will not add correctly via drag/drop until MDL-33053 is integrated too.

      Show
      Here's a scorm package that can be used: http://moodle.org/mod/data/view.php?d=50&rid=1655 This feature adds the ability to drag and drop scorm packages into a course Log in as an admin Enter a course and turn on editing drag a SCORM zip file over a section and choose to add as a SCORM package After SCORM has been added to the course, enter SCORM and make sure it loads correctly Please note there are a couple of known issues, please see the linked issues on MDL-22504 Also note that AICC Packages will not add correctly via drag/drop until MDL-33053 is integrated too.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      master_MDL-32937

      Description

      Davo (and others) have done some great work in MDL-22504 implementing drag/drop to add resources/folders etc to Moodle course pages, we should implement this for SCORM too.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Christopher Tombleson added a comment -

            I have implemented support for drag and drop for SCORM.

            https://github.com/chtombleson/moodle/tree/master-MDL-32937

            Show
            Christopher Tombleson added a comment - I have implemented support for drag and drop for SCORM. https://github.com/chtombleson/moodle/tree/master-MDL-32937
            Hide
            Dan Marsden added a comment -

            Thanks Chris - bouncing this for peer review - I haven't had time to look at this closely today but I'd like to get it in before 2.3 release if poss so submitting for peer review now.

            Show
            Dan Marsden added a comment - Thanks Chris - bouncing this for peer review - I haven't had time to look at this closely today but I'd like to get it in before 2.3 release if poss so submitting for peer review now.
            Hide
            Davo Smith added a comment -

            I've been through the code - it is basically sound, but there are a number of comments that I've made inline on GitHub (and I've only looked at the code, I haven't pulled it and tested it).

            Most of my comments relate to small coding methods, but there are a couple of issues that definitely need fixing:

            $scorm->intro should be '' (from MDL-334040)
            $scorm->cmidnumber should also be '' (not the course module id)

            I'd also recommend removing the temporary file from the unzipping (especially as it doesn't appear to get deleted if the package turns out to not be a SCORM).

            Maybe there is a case here for adding some method of reporting more useful error messages in cases where there are specific reasons why an activity couldn't be created (e.g. 'This zip file isn't actually a SCORM'), but I'm not quite sure how that mechanism would work.

            Show
            Davo Smith added a comment - I've been through the code - it is basically sound, but there are a number of comments that I've made inline on GitHub (and I've only looked at the code, I haven't pulled it and tested it). Most of my comments relate to small coding methods, but there are a couple of issues that definitely need fixing: $scorm->intro should be '' (from MDL-334040) $scorm->cmidnumber should also be '' (not the course module id) I'd also recommend removing the temporary file from the unzipping (especially as it doesn't appear to get deleted if the package turns out to not be a SCORM). Maybe there is a case here for adding some method of reporting more useful error messages in cases where there are specific reasons why an activity couldn't be created (e.g. 'This zip file isn't actually a SCORM'), but I'm not quite sure how that mechanism would work.
            Hide
            Dan Marsden added a comment -

            thanks for taking a look Davo - Chris can you please address the issues Davo raises? - thanks.

            Show
            Dan Marsden added a comment - thanks for taking a look Davo - Chris can you please address the issues Davo raises? - thanks.
            Hide
            Christopher Tombleson added a comment -

            I will have a look at the issues Davo raised this morning.

            Show
            Christopher Tombleson added a comment - I will have a look at the issues Davo raised this morning.
            Hide
            Christopher Tombleson added a comment -

            Hi,

            I have resolved the issues raised by Davo.

            Code is here: https://github.com/chtombleson/moodle/tree/master-MDL-32937

            Thanks

            Show
            Christopher Tombleson added a comment - Hi, I have resolved the issues raised by Davo. Code is here: https://github.com/chtombleson/moodle/tree/master-MDL-32937 Thanks
            Hide
            Dan Marsden added a comment -

            Hi Chris - just found another bug - can you make sure $scorm->scormtype is set to SCORM_TYPE_LOCAL when calling scorm_add_instance ? - it causes problems when not set.. especially when trying to upload an AICC package (see the file MDL_33053.zip on MDL-33053 for an AICC package to test)

            Show
            Dan Marsden added a comment - Hi Chris - just found another bug - can you make sure $scorm->scormtype is set to SCORM_TYPE_LOCAL when calling scorm_add_instance ? - it causes problems when not set.. especially when trying to upload an AICC package (see the file MDL_33053.zip on MDL-33053 for an AICC package to test)
            Hide
            Dan Marsden added a comment -

            hmmm - looks like your patch already does that - I should have checked... something else is causing a problem when adding aicc packages.

            Show
            Dan Marsden added a comment - hmmm - looks like your patch already does that - I should have checked... something else is causing a problem when adding aicc packages.
            Hide
            Martin Dougiamas added a comment -

            Woo! Keen to see this!

            Show
            Martin Dougiamas added a comment - Woo! Keen to see this!
            Hide
            Dan Marsden added a comment -

            I believe Chris has resolved the AICC issues but I think it required a few changes in the AICC core code which I haven't had a chance to review yet - and with teacher-only days and snow days getting in the way here I'm running out of time very quickly! - hopefully I'll get some time to look at it Thurs/Fri!

            Show
            Dan Marsden added a comment - I believe Chris has resolved the AICC issues but I think it required a few changes in the AICC core code which I haven't had a chance to review yet - and with teacher-only days and snow days getting in the way here I'm running out of time very quickly! - hopefully I'll get some time to look at it Thurs/Fri!
            Hide
            Dan Marsden added a comment -

            cool - thanks Chris - the AICC issues are actually resolved by MDL-33053

            Show
            Dan Marsden added a comment - cool - thanks Chris - the AICC issues are actually resolved by MDL-33053
            Hide
            Dan Marsden added a comment -

            pushing through for integration - thanks Chris!

            Show
            Dan Marsden added a comment - pushing through for integration - thanks Chris!
            Hide
            Matteo Scaramuccia added a comment - - edited

            What an amazing feature!

            # cd moodle-master
            # git status
            # git pull
            # mkdir ../dan-moodle-master
            # git clone -b master_MDL-32937 git://github.com/danmarsden/moodle.git ../dan-moodle-master/
            # cd ../dan-moodle-master/
            # git format-patch origin/master --stdout > ../master_MDL-32937.diff
            # vim ../master_MDL-32937.diff
            # cd ../moodle-master
            # git apply --stat ../master_MDL-32937.diff
            # git apply --check ../master_MDL-32937.diff
            # git am --signoff < ../master_MDL-32937.diff
            

            Note: shortcut for above:

            # cd moodle-master
            # git status
            # git pull
            # curl https://github.com/danmarsden/moodle/compare/master...master_MDL-32937.patch | git am --signoff
            

            Launched Chrome, logged in, selected a course in editing mode, picked up a SCORM test package (e.g. from MDL-33106, MDL-33106_v2_fixed), dragged and... voilà... D&D in 2.3 rocks and now it let Learning Packages (@Martin: lesson learned ) roll too.

            Great job Dan&Chris!
            Kudos to Davo (MDL-22504) too! I'm just realizing the impact of the D&D framework in Moodle 2.3: it shows me its full power in managing this improvement.

            +1

            Just a bunch of trivial potential improvements in Dan's pull:

            1. $scorm->sha1hash = sha1($file->get_filename()); could be

              $hash = $file->get_contenthash();
              ...
              $scorm->sha1hash = $hash;
              

              it will decrease the time required to finish the process by avoiding another SHA1 calculation;

            2. Should scorm_dndupload_handle() be mod_scorm_dndupload_handle(), see e.g. mod_folder_dndupload_handle()? The same should apply to scorm_dndupload_register. By design lib/moodlelib.php::get_plugin_list_with_function() accepts the current naming convention too;
              and finally (trivial^2):
            3. you're using part of the same code as in mod/scorm/mod_form.php::validation(): time to create a check_learning_package_type() returning {{ {'aicc', 'scorm', FALSE}

              }}?

            Show
            Matteo Scaramuccia added a comment - - edited What an amazing feature! # cd moodle-master # git status # git pull # mkdir ../dan-moodle-master # git clone -b master_MDL-32937 git://github.com/danmarsden/moodle.git ../dan-moodle-master/ # cd ../dan-moodle-master/ # git format-patch origin/master --stdout > ../master_MDL-32937.diff # vim ../master_MDL-32937.diff # cd ../moodle-master # git apply --stat ../master_MDL-32937.diff # git apply --check ../master_MDL-32937.diff # git am --signoff < ../master_MDL-32937.diff Note: shortcut for above: # cd moodle-master # git status # git pull # curl https://github.com/danmarsden/moodle/compare/master...master_MDL-32937.patch | git am --signoff Launched Chrome, logged in, selected a course in editing mode, picked up a SCORM test package (e.g. from MDL-33106 , MDL-33106 _v2_fixed), dragged and... voilà... D&D in 2.3 rocks and now it let Learning Packages (@Martin: lesson learned ) roll too. Great job Dan&Chris! Kudos to Davo ( MDL-22504 ) too! I'm just realizing the impact of the D&D framework in Moodle 2.3: it shows me its full power in managing this improvement. +1 Just a bunch of trivial potential improvements in Dan's pull: $scorm->sha1hash = sha1($file->get_filename()); could be $hash = $file->get_contenthash(); ... $scorm->sha1hash = $hash; it will decrease the time required to finish the process by avoiding another SHA1 calculation; Should scorm_dndupload_handle() be mod_scorm_dndupload_handle() , see e.g. mod_folder_dndupload_handle() ? The same should apply to scorm_dndupload_register . By design lib/moodlelib.php::get_plugin_list_with_function() accepts the current naming convention too; and finally (trivial^2): you're using part of the same code as in mod/scorm/mod_form.php::validation() : time to create a check_learning_package_type() returning {{ {'aicc', 'scorm', FALSE} }}?
            Hide
            Matteo Scaramuccia added a comment -

            Hi Dan,
            what about my proposal in MDL-33625:

            ...
                file_save_draft_area_files($uploadinfo->draftitemid, $context->id, 'mod_scorm', 'package', 0);
                $fs = get_file_storage();
                $file = $fs->get_file($contextid, 'mod_scorm', 'package', 0, '/', $uploadinfo->filename);
                $hash = $file->get_contenthash();
            ...
            

            ?

            Maybe I should do some tests but I'm doubting right know if there should be issues in selecting the right file with multiple packages in the same context and file area regardless the concurrency.

            Show
            Matteo Scaramuccia added a comment - Hi Dan, what about my proposal in MDL-33625 : ... file_save_draft_area_files($uploadinfo->draftitemid, $context->id, 'mod_scorm', 'package', 0); $fs = get_file_storage(); $file = $fs->get_file($contextid, 'mod_scorm', 'package', 0, '/', $uploadinfo->filename); $hash = $file->get_contenthash(); ... ? Maybe I should do some tests but I'm doubting right know if there should be issues in selecting the right file with multiple packages in the same context and file area regardless the concurrency.
            Hide
            Matteo Scaramuccia added a comment -

            Context ID is related with the Activity and not with the Course... ouch, more in MDL-33625... shame on Matteo . Left for the record.

            Show
            Matteo Scaramuccia added a comment - Context ID is related with the Activity and not with the Course... ouch, more in MDL-33625 ... shame on Matteo . Left for the record.
            Hide
            Dan Marsden added a comment -

            Thanks Matteo for the review!
            good spotting on sha1hash, but that shouldn't even be there as it is set by scorm_parse when detecting a new package and performs a few operations on the file - I've removed that now.

            have renamed the functions too - looks like the naming was updated since we started on the patch, thanks!

            I'd really like to improve the way we validate packages quite a bit - have opened MDL-33833 for this but as we're soo close to 2.3 release I don't really want to introduce changes to other areas of SCORM that would cause increased load on the testing/integrators etc but definitely something to do for 2.4!

            Show
            Dan Marsden added a comment - Thanks Matteo for the review! good spotting on sha1hash, but that shouldn't even be there as it is set by scorm_parse when detecting a new package and performs a few operations on the file - I've removed that now. have renamed the functions too - looks like the naming was updated since we started on the patch, thanks! I'd really like to improve the way we validate packages quite a bit - have opened MDL-33833 for this but as we're soo close to 2.3 release I don't really want to introduce changes to other areas of SCORM that would cause increased load on the testing/integrators etc but definitely something to do for 2.4!
            Hide
            Dan Marsden added a comment -

            heh - just found MDL-33400 - have removed the mod_ prefix from the dnd function names.

            Show
            Dan Marsden added a comment - heh - just found MDL-33400 - have removed the mod_ prefix from the dnd function names.
            Hide
            Matteo Scaramuccia added a comment -

            OK
            BTW, MDL-33833 doesn't exist... yet

            => MDL-33633

            Show
            Matteo Scaramuccia added a comment - OK BTW, MDL-33833 doesn't exist... yet => MDL-33633
            Hide
            Dan Poltawski added a comment -

            Thanks everyone! Fab to see.

            I did one minor commit to switch the two inline comments to // to match coding style

            Show
            Dan Poltawski added a comment - Thanks everyone! Fab to see. I did one minor commit to switch the two inline comments to // to match coding style
            Hide
            Dan Poltawski added a comment -

            Oh, a tiny comment - I wasn't sure about the phrase 'Create SCORM package' - that seemed like you would be authoring a scorm package.

            Show
            Dan Poltawski added a comment - Oh, a tiny comment - I wasn't sure about the phrase 'Create SCORM package' - that seemed like you would be authoring a scorm package.
            Hide
            Dan Poltawski added a comment -

            Worked perfectly.

            My only question is whether it should be 'Create a SCORM resource' or something like that. Its minor though.

            Show
            Dan Poltawski added a comment - Worked perfectly. My only question is whether it should be 'Create a SCORM resource' or something like that. Its minor though.
            Hide
            Dan Poltawski added a comment -

            (not sure if 'create a resource' is clearer)

            Show
            Dan Poltawski added a comment - (not sure if 'create a resource' is clearer)
            Hide
            Dan Marsden added a comment -

            thanks Dan I see what you mean.

            "SCORM package" is the new pluginname that MD bestowed upon the SCORM mod recently which I quite like. we probably can't change that too much

            I used the word "Create" to be consistent with the other modules in the drag/drop interface but maybe we could change that to "Add" so it becomes "Add SCORM Package" - do you think that makes more sense?

            Show
            Dan Marsden added a comment - thanks Dan I see what you mean. "SCORM package" is the new pluginname that MD bestowed upon the SCORM mod recently which I quite like. we probably can't change that too much I used the word "Create" to be consistent with the other modules in the drag/drop interface but maybe we could change that to "Add" so it becomes "Add SCORM Package" - do you think that makes more sense?
            Hide
            Dan Poltawski added a comment -

            Hmm, could it be 'Add as SCORM package'? I'm not sure really.

            Adding Helen here as she usually has good ideas

            Show
            Dan Poltawski added a comment - Hmm, could it be 'Add as SCORM package'? I'm not sure really. Adding Helen here as she usually has good ideas
            Hide
            Helen Foster added a comment -

            I'd suggest 'Add a SCORM package'.

            There are lots of other places where the word 'Add' is used - 'Add an activity or resource', 'Add a block' etc and I agree with Dan that 'Add' is better than 'Create' to avoid confusion about authoring the package.

            Show
            Helen Foster added a comment - I'd suggest 'Add a SCORM package'. There are lots of other places where the word 'Add' is used - 'Add an activity or resource', 'Add a block' etc and I agree with Dan that 'Add' is better than 'Create' to avoid confusion about authoring the package.
            Hide
            Matteo Scaramuccia added a comment -

            In case you were interested in my opinion: +1 to "Add as SCORM package"; this implies a fix for the other modules too.
            The same should apply in case you select "Add" instead of "Create" i.e. w/o "as ":

            $ grep dndupload mod/*/lang/en/*.php | grep -i create
            mod/folder/lang/en/folder.php:$string['dnduploadmakefolder'] = 'Unzip files and create folder';
            mod/resource/lang/en/resource.php:$string['dnduploadresource'] = 'Create file resource';
            

            Show
            Matteo Scaramuccia added a comment - In case you were interested in my opinion: +1 to "Add as SCORM package"; this implies a fix for the other modules too. The same should apply in case you select "Add" instead of "Create" i.e. w/o "as ": $ grep dndupload mod/*/lang/en/*.php | grep -i create mod/folder/lang/en/folder.php:$string['dnduploadmakefolder'] = 'Unzip files and create folder'; mod/resource/lang/en/resource.php:$string['dnduploadresource'] = 'Create file resource';
            Hide
            Dan Marsden added a comment -

            have pushed a commit to the above repo that changes the string to "Add a SCORM package" - will leave it to someone else to report bug/fix lang in other modules. - thanks all.

            Show
            Dan Marsden added a comment - have pushed a commit to the above repo that changes the string to "Add a SCORM package" - will leave it to someone else to report bug/fix lang in other modules. - thanks all.
            Hide
            Dan Poltawski added a comment -

            Thanks Dan, i've pulled that lang string change in.

            Show
            Dan Poltawski added a comment - Thanks Dan, i've pulled that lang string change in.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL

            Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Big thanks for the effort. This is now part of Moodle upstream. Let's wait for regressions, yay! LOL Ciao
            Hide
            Helen Foster added a comment -

            Thanks a lot for this nice improvement to Moodle 2.3. Documentation about it is now available:

            Show
            Helen Foster added a comment - Thanks a lot for this nice improvement to Moodle 2.3. Documentation about it is now available: http://docs.moodle.org/23/en/SCORM_settings http://docs.moodle.org/23/en/Course_homepage#Drag_and_drop_method
            Hide
            Martin Dougiamas added a comment -

            Woot!

            Show
            Martin Dougiamas added a comment - Woot!
            Hide
            Dan Marsden added a comment -

            thanks Helen!

            Show
            Dan Marsden added a comment - thanks Helen!

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: