Moodle

External SCORM manifest URL checks only for syntax

Details

  • Testing Instructions:
    Hide
    1. Get a SCORM Package (zip file)
    2. Unzip it and place it in a folder reachable by HTTP (you can leave the zip itself within that folder)
    3. Take note of the URL pointing to the imsmanifest.xml file , supposing here to be http://hostname/path/to/resource.imsmanifest.xml
    4. Enable allowtypeexternal* in the SCORM Activity settings (/admin/settings.php?section=modsettingscorm)
    5. Create a SCORM activity selecting the Type among the External ones (AICC included)
    6. Paste the URL above in URL field
      • Add a suffix to the URL e.g. .external and click on Save and display: expect Invalid URL specified
      • Change something in the URL appending e.g. .external to /path (=>/path.external/} and click on Save and display: expect Invalid URL specified. Debug message: followed by a preformatted text telling what's going on
    7. Change the Type with the other External and try again the previous step
    8. Select Type equal to External SCORM manifest
    9. Paste the URL, the right one
    10. Click on Save and display: expect the package having been published, play with it just to see the TOC and click on one of the items
    Show
    Get a SCORM Package (zip file) Unzip it and place it in a folder reachable by HTTP (you can leave the zip itself within that folder) Take note of the URL pointing to the imsmanifest.xml file , supposing here to be http://hostname/path/to/resource.imsmanifest.xml Enable allowtypeexternal* in the SCORM Activity settings ( /admin/settings.php?section=modsettingscorm ) Create a SCORM activity selecting the Type among the External ones (AICC included) Paste the URL above in URL field Add a suffix to the URL e.g. .external and click on Save and display : expect Invalid URL specified Change something in the URL appending e.g. .external to /path (=> /path.external/ } and click on Save and display : expect Invalid URL specified. Debug message: followed by a preformatted text telling what's going on Change the Type with the other External and try again the previous step Select Type equal to External SCORM manifest Paste the URL, the right one Click on Save and display : expect the package having been published, play with it just to see the TOC and click on one of the items
  • Affected Branches:
    MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
  • Fixed Branches:
    MOODLE_24_STABLE
  • Pull from Repository:
  • Pull Master Branch:
    m24_MDL-36641_Missing_external_package_HTTP_check
  • Pull Master Diff URL:

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Matteo Scaramuccia added a comment -

Here is my patch proposal: it makes usage of an HTTP HEAD call which is used in few other place so it's safe to adopt it here too. The debug message cannot be simplified into a set of predefined error messages, to help the troubleshooting.

Now, I'll look at the issue in the forum (where everything started): it could be this issue not related with the original post but with Dan's reply.

Show
Matteo Scaramuccia added a comment - Here is my patch proposal: it makes usage of an HTTP HEAD call which is used in few other place so it's safe to adopt it here too. The debug message cannot be simplified into a set of predefined error messages, to help the troubleshooting. Now, I'll look at the issue in the forum (where everything started): it could be this issue not related with the original post but with Dan's reply.
Hide
Permalink
Matteo Scaramuccia added a comment -

Fixed a small typo (use => used) in a comment inside scorm_check_url().

Show
Matteo Scaramuccia added a comment - Fixed a small typo ( use => used ) in a comment inside scorm_check_url() .
Hide
Permalink
Matteo Scaramuccia added a comment -

Damn' flu! Just realized that:

  1. the branch name was incorrect
  2. more important the coding style for the nested if was tremendous : 
    +            } else {
+                // Availability check.
+                if ($errormsg = scorm_check_url($reference)) {
+                    $errors['packageurl'] = $errormsg;
+                }
             }
Show
Matteo Scaramuccia added a comment - Damn' flu! Just realized that: the branch name was incorrect more important the coding style for the nested if was tremendous : + } else { + // Availability check. + if ($errormsg = scorm_check_url($reference)) { + $errors['packageurl'] = $errormsg; + } }
Hide
Permalink
Dan Marsden added a comment -

thanks Matteo - took a quick look and it looks ok - couple of extra carriage returns in the patch that might not be needed but I haven't actually looked at the full code so it might make sense to have those in the patch? - I'll try to do a proper peer review on this later this week - if I haven't done it by Thurs ping me again to take a look!

Show
Dan Marsden added a comment - thanks Matteo - took a quick look and it looks ok - couple of extra carriage returns in the patch that might not be needed but I haven't actually looked at the full code so it might make sense to have those in the patch? - I'll try to do a proper peer review on this later this week - if I haven't done it by Thurs ping me again to take a look!
Hide
Permalink
Matteo Scaramuccia added a comment -

Ping!
L2R419 and L2R429 are there according to the style used for the previous check "blocks" i.e added intentionally.

Show
Matteo Scaramuccia added a comment - Ping! L2R419 and L2R429 are there according to the style used for the previous check "blocks" i.e added intentionally.
Hide
Permalink
Dan Marsden added a comment -

looking again..

not sure what I think about returning false on success? - my personal preference for functions would be to return true when no error is found - although I'm not sure what the integrators think about that.

Show
Dan Marsden added a comment - looking again.. not sure what I think about returning false on success? - my personal preference for functions would be to return true when no error is found - although I'm not sure what the integrators think about that.
Hide
Permalink
Dan Marsden added a comment -

yeah - just had a chat with Sam H - returning true or error string would be more consistent with the rest of Moodle rather than false or error string - would you mind changing that? - thanks!

Show
Dan Marsden added a comment - yeah - just had a chat with Sam H - returning true or error string would be more consistent with the rest of Moodle rather than false or error string - would you mind changing that? - thanks!
Hide
Permalink
Matteo Scaramuccia added a comment -

OK
false|string was selected to let the code consuming be compact enough: BTW, I'll do the required changes.

Show
Matteo Scaramuccia added a comment - OK false|string was selected to let the code consuming be compact enough: BTW, I'll do the required changes.
Hide
Permalink
Matteo Scaramuccia added a comment -

Hi Dan,
here is the code using true|string, rebased against 2.4beta+ (Build: 20121123).

Show
Matteo Scaramuccia added a comment - Hi Dan, here is the code using true|string , rebased against 2.4beta+ (Build: 20121123) .
Hide
Permalink
Dan Marsden added a comment -

cool - looks ok to me - bouncing up for integration. not completely sure about the is_string() stuff but will leave the integrator to make a call on that. Thanks Matteo!

Show
Dan Marsden added a comment - cool - looks ok to me - bouncing up for integration. not completely sure about the is_string() stuff but will leave the integrator to make a call on that. Thanks Matteo!
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Integrated (24 and master).

I don't think we should be backporting this to older stables, as far as may be considered an improvement.

Thanks!

Show
Eloy Lafuente (stronk7) added a comment - Integrated (24 and master). I don't think we should be backporting this to older stables, as far as may be considered an improvement. Thanks!
Hide
Permalink
Ankit Agarwal added a comment -

works as described.
passing
Thanks

Show
Ankit Agarwal added a comment - works as described. passing Thanks
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

Closing, ciao

Show
Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

People

Vote (1)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: