Moodle
  1. Moodle
  2. MDL-33053

Incorrect Table Of Content hierarchy using "structured" AICC courses

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.18, 2.2.3
    • Fix Version/s: 2.2.4, 2.3.1
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide
      1. (Opt) Create an empty course;
      2. Add a new SCORM/AICC activity using the attached package, MDL_33053.zip. It must be NEW for each test run: at least before and after applying the commit. The package must be re-published to allow the patch to store the missing items into the DB;
      3. Click on an item presented in the TOC area of the Moodle Player: the hierarchy presented in the selected item content must be correctly represented in the TOC area of the Moodle Player;
      4. [Only 2.x] Please test some SCORM packages to highlight any potential regression: the code for the Player TOC is actually shared among AICC and SCORM 1.2/2004 packages. This is not true for 1.9. SCORM 1.2 package similar to MDL_33053.zip: MDL-33053_SCORM_1p2.zip
      Show
      (Opt) Create an empty course; Add a new SCORM/AICC activity using the attached package, MDL_33053.zip. It must be NEW for each test run: at least before and after applying the commit. The package must be re-published to allow the patch to store the missing items into the DB; Click on an item presented in the TOC area of the Moodle Player: the hierarchy presented in the selected item content must be correctly represented in the TOC area of the Moodle Player; [Only 2.x] Please test some SCORM packages to highlight any potential regression: the code for the Player TOC is actually shared among AICC and SCORM 1.2/2004 packages. This is not true for 1.9. SCORM 1.2 package similar to MDL_33053.zip: MDL-33053 _SCORM_1p2.zip
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      m23_MDL-33053_AICC_flattened_TOC
    • Rank:
      40233

      Description

      When the organization of the AUs is based on a hierarchy of chapters, Moodle will show the AUs to the user using a flattened 1st level tree i.e. w/o the original hierarchy described in the CST file.

        Issue Links

          Activity

          Hide
          Matteo Scaramuccia added a comment -

          The sample package attached here, MDL_33053.zip - being not a real AICC course since each AU is not able to track anything - , shows the issue and it can be used for testing the patch proposal I'm going to publish.

          Show
          Matteo Scaramuccia added a comment - The sample package attached here, MDL_33053.zip - being not a real AICC course since each AU is not able to track anything - , shows the issue and it can be used for testing the patch proposal I'm going to publish.
          Hide
          Dan Marsden added a comment -

          thanks Matteo - that's a known issue that I'm hoping to fix as part of MDL-32835

          Show
          Dan Marsden added a comment - thanks Matteo - that's a known issue that I'm hoping to fix as part of MDL-32835
          Hide
          Dan Marsden added a comment -

          ..at least I presume it's the same issue without looking at your patch or the package

          Show
          Dan Marsden added a comment - ..at least I presume it's the same issue without looking at your patch or the package
          Hide
          Matteo Scaramuccia added a comment -

          2.2 is affected too: in the next days I'll go deeper in the 2.x code guessing that the tree of the new viewer will be able to correctly HTMLize the hierarchy when all the items in the DES file will be imported into the database.

          Show
          Matteo Scaramuccia added a comment - 2.2 is affected too: in the next days I'll go deeper in the 2.x code guessing that the tree of the new viewer will be able to correctly HTMLize the hierarchy when all the items in the DES file will be imported into the database.
          Hide
          Matteo Scaramuccia added a comment -

          Hi Dan,
          you're too fast Here is late night (1.03am) and I'm slowing down my keyboard typing

          BTW I'm not sure that MDL-32835 tells about the same issue: here the problem is strictly related with AICC packages, being incorrectly imported into the database AND with the TOC creation code confusing 0 and false in the array_search() return value (see the commit comment too).

          Show
          Matteo Scaramuccia added a comment - Hi Dan, you're too fast Here is late night (1.03am) and I'm slowing down my keyboard typing BTW I'm not sure that MDL-32835 tells about the same issue: here the problem is strictly related with AICC packages, being incorrectly imported into the database AND with the TOC creation code confusing 0 and false in the array_search() return value (see the commit comment too).
          Hide
          Dan Marsden added a comment -

          hehe, looks like it's quite a different issue. we can't fix this in 1.9 until it's fixed upstream in master, 2.3 and 2.2 - don't need to look at 2.0 and support for bug fixes in 2.1 ends in June so we can probably ignore 2.1 depending on when we get patches for the other versions ready.

          Show
          Dan Marsden added a comment - hehe, looks like it's quite a different issue. we can't fix this in 1.9 until it's fixed upstream in master, 2.3 and 2.2 - don't need to look at 2.0 and support for bug fixes in 2.1 ends in June so we can probably ignore 2.1 depending on when we get patches for the other versions ready.
          Hide
          Matteo Scaramuccia added a comment -

          OK
          Time to sleep now
          I'll start w/ 2.2 in the next days, providing you the fix for both 2.2 and 2.3 branches.

          Show
          Matteo Scaramuccia added a comment - OK Time to sleep now I'll start w/ 2.2 in the next days, providing you the fix for both 2.2 and 2.3 branches.
          Hide
          Dan Marsden added a comment -

          cool - thanks Matteo!

          Show
          Dan Marsden added a comment - cool - thanks Matteo!
          Hide
          Michael de Raadt added a comment -

          Thanks for working on this.

          Show
          Michael de Raadt added a comment - Thanks for working on this.
          Hide
          Matteo Scaramuccia added a comment - - edited

          On fixing the issue in 2.2 I've seen an - unrelated - issue in the navigation panel which gives a different user experience compared with the one in 1.9. I'm also working on it => module.js:

          • (scorm_current_node).title == null is the key point;
          • missing of focus on selecting an item;
          • meaning of '<<' and '>>'.

          @Dan: what should be the expected behaviour of "<<" and ">>"? I'm working at:

          • "<" and ">": per-item (AU/SCO) navigation i.e. previous and next at leaf lowest level in terms of the tree representing the course structure, being the user able to get the previous/next item i.e. jumping into the sibling, at the parent level, subtree. Same as in 1.9;
          • "<<" and ">>", my doubts:
            a. moving between first and last among the leaves in the tree, like for the pagination in a result set
            b. moving to the previous/next leaf in the first sibling - compared to the tree of the current (selected) item - subtree
            c. ???

          Guessing the navigation panel issue should be managed in another bug: give me just some hints and then I'll start a brand new issue. Here it is: MDL-33106.

          Note: +1 for MDL-32794, which allows parent/folder items to be toggled instead of being selected.

          Show
          Matteo Scaramuccia added a comment - - edited On fixing the issue in 2.2 I've seen an - unrelated - issue in the navigation panel which gives a different user experience compared with the one in 1.9. I'm also working on it => module.js : (scorm_current_node).title == null is the key point; missing of focus on selecting an item; meaning of '<<' and '>>'. @Dan: what should be the expected behaviour of "<<" and ">>"? I'm working at: "<" and ">": per-item (AU/SCO) navigation i.e. previous and next at leaf lowest level in terms of the tree representing the course structure, being the user able to get the previous/next item i.e. jumping into the sibling, at the parent level, subtree. Same as in 1.9; "<<" and ">>", my doubts: a. moving between first and last among the leaves in the tree, like for the pagination in a result set b. moving to the previous/next leaf in the first sibling - compared to the tree of the current (selected) item - subtree c. ??? Guessing the navigation panel issue should be managed in another bug: give me just some hints and then I'll start a brand new issue. Here it is: MDL-33106 . Note: +1 for MDL-32794 , which allows parent/folder items to be toggled instead of being selected.
          Hide
          Matteo Scaramuccia added a comment -

          In the next days:

          1. I'll provide a SCORM 1.2 packaged version of the AICC package included here;
          2. I'll provide a git commit for 2.3 (current master);
          3. A proposal for the navigation panel, pending the clarification about '<<' and '>>', named in the code "Skip Prev" and "Skip Next".
          Show
          Matteo Scaramuccia added a comment - In the next days: I'll provide a SCORM 1.2 packaged version of the AICC package included here; I'll provide a git commit for 2.3 (current master); A proposal for the navigation panel, pending the clarification about '<<' and '>>', named in the code "Skip Prev" and "Skip Next".
          Hide
          Dan Marsden added a comment -

          looks good - passing peer review, thanks Matteo.

          Show
          Dan Marsden added a comment - looks good - passing peer review, thanks Matteo.
          Hide
          Dan Marsden added a comment -

          I'm approving this only for the 2.2/master branches at this stage - not 1.9Stable so pasting Matteo's 1.9 patch here if anyone wants to apply it on their local installs (and removing it from the integration fields above)
          https://github.com/scara/moodle/commit/35acd75dcceb12142dec2c7d288182a8d2b2f326

          If there's a large number of 1.9 users wanting this applied to the 1.9stable branch feel free to create a new tracker issue for it but I'm unlikely to look at pushing it for integration unless it gets a bunch of votes.

          Show
          Dan Marsden added a comment - I'm approving this only for the 2.2/master branches at this stage - not 1.9Stable so pasting Matteo's 1.9 patch here if anyone wants to apply it on their local installs (and removing it from the integration fields above) https://github.com/scara/moodle/commit/35acd75dcceb12142dec2c7d288182a8d2b2f326 If there's a large number of 1.9 users wanting this applied to the 1.9stable branch feel free to create a new tracker issue for it but I'm unlikely to look at pushing it for integration unless it gets a bunch of votes.
          Hide
          Dan Marsden added a comment -

          NOTE TO INTEGRATOR: Master and 22_STABLE only please - thanks!

          Show
          Dan Marsden added a comment - NOTE TO INTEGRATOR: Master and 22_STABLE only please - thanks!
          Hide
          Matteo Scaramuccia added a comment -

          @Integrators: MDL-32937 has been integrated and it should be nice if MDL-33053 could be integrated before releasing 2.3 too; it will allow AICC Packages to work with DnD upload.

          Show
          Matteo Scaramuccia added a comment - @Integrators: MDL-32937 has been integrated and it should be nice if MDL-33053 could be integrated before releasing 2.3 too; it will allow AICC Packages to work with DnD upload.
          Hide
          Dan Marsden added a comment -

          Thanks Matteo - at this late stage before 2.3 release I actually think it's probably better if the integrators focus on the other issues in the integration backlog at the moment - the number of people using AICC in Moodle isn't as large as the number of people that will be affected by most of the other issues in the integration queue at the moment. I'm quite happy for this to sit for a couple of weeks if needed.

          Show
          Dan Marsden added a comment - Thanks Matteo - at this late stage before 2.3 release I actually think it's probably better if the integrators focus on the other issues in the integration backlog at the moment - the number of people using AICC in Moodle isn't as large as the number of people that will be affected by most of the other issues in the integration queue at the moment. I'm quite happy for this to sit for a couple of weeks if needed.
          Hide
          Matteo Scaramuccia added a comment -

          OK
          Mine was just a reminder: not sure if it was better to put kind of link to MDL-32937.

          Show
          Matteo Scaramuccia added a comment - OK Mine was just a reminder: not sure if it was better to put kind of link to MDL-32937 .
          Hide
          Dan Poltawski added a comment -

          Thanks Matteo and Dan, i've integrated this to 22, 23 and master.

          Show
          Dan Poltawski added a comment - Thanks Matteo and Dan, i've integrated this to 22, 23 and master.
          Hide
          Frédéric Massart added a comment -

          Test passed on 2.2, 2.3 and master. Yay!

          Show
          Frédéric Massart added a comment - Test passed on 2.2, 2.3 and master. Yay!
          Hide
          Sam Hemelryk added a comment -

          Congratulations your code is upstream - gold star for you!

          This issue + 79 others made it in in time for the minor releases.
          Thank you everyone involved for your exuberant efforts.

          Show
          Sam Hemelryk added a comment - Congratulations your code is upstream - gold star for you! This issue + 79 others made it in in time for the minor releases. Thank you everyone involved for your exuberant efforts.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: