Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.4
    • Component/s: SCORM
    • Labels:
    • Rank:
      39866

      Description

      scorm_get_toc needs a rewrite as it doesn't always generate valid html for complex scorm packages and doesn't contain the information in a easy to parse manner for other functions (like seq/nav for SCORM 2004)

      We need at least 3 new functions.
      scorm_get_toc_object()
      this should be based on the existing scorm_get_toc but instead of generating html, it should generate an object that can be parsed to create the html - this function should take the data from a call to scorm_get_scoes and return each sco in a formatted object with these extra params:

      $sco->url = ''; //this isn't a full url, just the bits required to link to the sco - this is what is inserted into the 'title' tag in the yui treeview.
      $sco->statusicon = '<img>'; //this is the status icon that should be displayed with the sco
      $sco->children = array() - this contains an array of any children $scos formatted in a similar way.

      Then a separate function
      scorm_format_toc_for_treeview()
      which takes the output of scorm_get_toc_object() and generates the html used by the YUI treeview object.

      Then a separate function
      scorm_format_toc_for_droplist()
      which generates a drop list used by $result->tocmenu for the drop list nav item.

      Finally another function scorm_get_toc_new() - this function should return the exact same information as the existing scorm_get_toc() but using the new functions above so we can easily swap between using the old scorm_get_toc() and the new scorm_get_toc_new()

      these functions should all be in mod/scorm/locallib.php and the changes shouldn't modify any existing functions. It is preferred if the existing functions don't do what is required then we duplicate that existing function under a new name and change the "new" copy of that function. Doing this means we can compare the existing scorm_get_toc with the new code to make sure the new one operates as expected.

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

          adding Mayank as a watcher here - Mayank I'm hoping to get one of the devs here at Catalyst to take a look at this before GSOC starts - I'm hoping it might help to resolve some of the navigation issues we're having as well as the invalid html created occasionally.

          Show
          Dan Marsden added a comment - adding Mayank as a watcher here - Mayank I'm hoping to get one of the devs here at Catalyst to take a look at this before GSOC starts - I'm hoping it might help to resolve some of the navigation issues we're having as well as the invalid html created occasionally.
          Hide
          Christopher Tombleson added a comment -
          Show
          Christopher Tombleson added a comment - I have fixed the issue here: https://github.com/chtombleson/moodle/tree/master-MDL-32835
          Hide
          Luis Estéfano added a comment -

          Thanks for your work Christopher!
          I don't know what it fix exactly, but the issue MDL-32794 is still appearing. So I think it doesn't fix all the problems with the complex tree displaying.

          Show
          Luis Estéfano added a comment - Thanks for your work Christopher! I don't know what it fix exactly, but the issue MDL-32794 is still appearing. So I think it doesn't fix all the problems with the complex tree displaying.
          Hide
          Dan Marsden added a comment -

          Hi Luis - Chris's patch isn't quite complete(on purpose) - you will need to change all instances of scorm_get_toc with scorm_get_toc_new after applying his patch

          I haven't tested the patch yet, but it will still cause your Toc to change slightly when you click on items in your toc that don't have related scos but it shouldn't generate invalid html causing the list to remove some of your items. To fix this we need to prevent those items in the tree from being clickable.

          Show
          Dan Marsden added a comment - Hi Luis - Chris's patch isn't quite complete(on purpose) - you will need to change all instances of scorm_get_toc with scorm_get_toc_new after applying his patch I haven't tested the patch yet, but it will still cause your Toc to change slightly when you click on items in your toc that don't have related scos but it shouldn't generate invalid html causing the list to remove some of your items. To fix this we need to prevent those items in the tree from being clickable.
          Hide
          Dan Marsden added a comment -

          heh - sorry - I see Chris does that in his patch already! - cool! - I'll be taking a look at this today.

          Show
          Dan Marsden added a comment - heh - sorry - I see Chris does that in his patch already! - cool! - I'll be taking a look at this today.
          Hide
          Dan Marsden added a comment -

          FYI to anyone else looking at this bug - Chris's patch above isn't quite right yet and has a few issues and I think he's away for a week so this probably won't see any further updates for at least another week.

          Show
          Dan Marsden added a comment - FYI to anyone else looking at this bug - Chris's patch above isn't quite right yet and has a few issues and I think he's away for a week so this probably won't see any further updates for at least another week.
          Hide
          Dan Marsden added a comment -

          Mayank (GSOC Student) has tidied Chris's patch up a bit - I haven't had a close look but putting this up for peer review - would be nice if someone else had time to look at this and give feedback

          Show
          Dan Marsden added a comment - Mayank (GSOC Student) has tidied Chris's patch up a bit - I haven't had a close look but putting this up for peer review - would be nice if someone else had time to look at this and give feedback
          Hide
          Dan Poltawski added a comment -

          Hi Guys,

          Thanks for your work on this. First a big disclaimer - i'm not very familiar with SCORM at all, so take my feedback with a pinch of salt. I'm just giving my point of view, you don't necessairly have to follow it

          • In general, I think this function could really benefit by being further split down. Its really difficult to keep track of all the different variables in this function, work out if they are being properly initialised etc.
            • For an example of that, look at $incomplete variable, its being initialised outside of the main loop but checked within the loop and its not clear to me whether the intention is for that to be reset to false every loop iteration (in which case there is a bug) or its supposed to exist persistently.
          • This doesn't look right:
            $sco->title = $sco->title;
          • {$statusicon} is being set multiple times down the body of the loop, but its not appended, so the last one wins.
          • You could use the html_writer:: class for writing the images (perhaps that isn't widespread in scorm so isn't sensible)
          • There are a few tiny coding style whitespace issues, running the Moodle code checker would find them, like:
            scorm_get_tracks($sco->id,$user->id, $attempt))
            
          • I didn't know the signifiance of the identifier 'scormtree123'
          • I'm not sure if the $SESSSION->scorm use is appropiate

          cheers,
          dan

          Show
          Dan Poltawski added a comment - Hi Guys, Thanks for your work on this. First a big disclaimer - i'm not very familiar with SCORM at all, so take my feedback with a pinch of salt. I'm just giving my point of view, you don't necessairly have to follow it In general, I think this function could really benefit by being further split down. Its really difficult to keep track of all the different variables in this function, work out if they are being properly initialised etc. For an example of that, look at $incomplete variable, its being initialised outside of the main loop but checked within the loop and its not clear to me whether the intention is for that to be reset to false every loop iteration (in which case there is a bug) or its supposed to exist persistently. This doesn't look right: $sco->title = $sco->title; {$statusicon} is being set multiple times down the body of the loop, but its not appended, so the last one wins. You could use the html_writer:: class for writing the images (perhaps that isn't widespread in scorm so isn't sensible) There are a few tiny coding style whitespace issues, running the Moodle code checker would find them, like: scorm_get_tracks($sco->id,$user->id, $attempt)) I didn't know the signifiance of the identifier 'scormtree123' I'm not sure if the $SESSSION->scorm use is appropiate cheers, dan
          Hide
          Dan Marsden added a comment -

          thanks Dan - that's great feedback.

          I've wanted to dump the $SESSION stuff for a while - my "big" plan is to rewrite a chunk of SCORM to use a class and use params inside the class to store that stuff - but that is probably outside the scope of the GSOC project I think... although when 2.3 is branched I might do some of that work myself.

          Mayank - could you please take a look to see if there are any easy ways you could break the function down further? - thanks!

          Show
          Dan Marsden added a comment - thanks Dan - that's great feedback. I've wanted to dump the $SESSION stuff for a while - my "big" plan is to rewrite a chunk of SCORM to use a class and use params inside the class to store that stuff - but that is probably outside the scope of the GSOC project I think... although when 2.3 is branched I might do some of that work myself. Mayank - could you please take a look to see if there are any easy ways you could break the function down further? - thanks!
          Hide
          Dan Marsden added a comment -

          This patch was included in fix for MDL-35418

          Show
          Dan Marsden added a comment - This patch was included in fix for MDL-35418

            People

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

              Dates

              • Created:
                Updated:
                Resolved: