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
    • Rank:
      40016

      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.

        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: