Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.4, 2.4.5, 2.5.1
    • Fix Version/s: 2.4.6, 2.5.2
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      Test 1:
      1.Add an AICC package to a course before this patch (contact Dan for packages if you want more packages for testing but one is attached here)
      2. Upgrade the site using the patch
      3. Enter AICC package as a student and make sure it works and no PHP Strict warning errors or other PHP errors occur.

      Test 2:
      On upgraded site add a new AICC pacakge
      1. Use Attached ZIP and load as SCORM (it is AICC course)
      2. Enter SCORM as a student and make sure the TOC and content loads.

      Show
      Test 1: 1.Add an AICC package to a course before this patch (contact Dan for packages if you want more packages for testing but one is attached here) 2. Upgrade the site using the patch 3. Enter AICC package as a student and make sure it works and no PHP Strict warning errors or other PHP errors occur. Test 2: On upgraded site add a new AICC pacakge 1. Use Attached ZIP and load as SCORM (it is AICC course) 2. Enter SCORM as a student and make sure the TOC and content loads.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
      git@github.com:danmarsden/moodle.git
    • Pull 2.4 Branch:
      m24_MDL-37393
    • Pull 2.5 Branch:
      m25_MDL-37393
    • Pull Master Branch:
      master_MDL-37393
    • Rank:
      47016

      Description

      AICC Content loaded using Metadata fail to launch.

      Same content launches without issue in 2.3.2

      The issue appears to be in the scorm_get_toc_get_parent_child() function on the locallib not correctly generating the child relationships so the TOC is not created correctly and so the SCORM player does not generate TOC correctly, and thus does not trigger the load of the loadsco page.

      STEPS
      1. Use Attached ZIP and load as SCORM (it is AICC course)
      2. Launch and TOC in SCORM Player will NOT display correct course structure and wont load.

      PROOF ISSUE IS WITH TOC CODE
      Edit database tables and locate the entries for the AICC course
      Edit row where orgization is mo_olpu_a02_dt_enus and identifier is A0
      Set parent = mo_olpu_a02_dt_enus from /

      NOTE: Imported data is identical in 2.3.2 so we know issue is NOT with AICC parsing

      1. screenshot-1.jpg
        95 kB
      2. screenshot-2.jpg
        71 kB
      3. screenshot-3.jpg
        30 kB
      4. screenshot-4.jpg
        40 kB

        Issue Links

          Activity

          Hide
          Martin Holden added a comment -

          AICC Test content

          Show
          Martin Holden added a comment - AICC Test content
          Hide
          Martin Holden added a comment -

          Illustrate the incorrect TOC and the JavaScript error when launching

          Show
          Martin Holden added a comment - Illustrate the incorrect TOC and the JavaScript error when launching
          Hide
          Martin Holden added a comment -

          Data in scorm_scoes table after importing sample course.

          Show
          Martin Holden added a comment - Data in scorm_scoes table after importing sample course.
          Hide
          Martin Holden added a comment -

          Showing hand editted database (to work around bug) as data is CORRECT as it works fine with these entries in 2.3.2

          Show
          Martin Holden added a comment - Showing hand editted database (to work around bug) as data is CORRECT as it works fine with these entries in 2.3.2
          Hide
          Martin Holden added a comment -

          After edit to database records we can see content loads and the TOC is displayed correctly.

          Show
          Martin Holden added a comment - After edit to database records we can see content loads and the TOC is displayed correctly.
          Hide
          Dan Marsden added a comment -

          looks like the new code expects a single sco with a parent of / - if multiple scos use / as the parent it gets a bit stuck.

          I think this migh be a bug with the code in datamodels/aicclib.php that the old toc code worked around but I need to confirm/test this a bit better - fixing it in aicclib.php will mean we will need to write an upgrade script to fix any old invalid records as well.

          I'd also be interested to see if the new code copes with multiple org's in a package - Matteo do you have any packages that use multiple orgs?

          Show
          Dan Marsden added a comment - looks like the new code expects a single sco with a parent of / - if multiple scos use / as the parent it gets a bit stuck. I think this migh be a bug with the code in datamodels/aicclib.php that the old toc code worked around but I need to confirm/test this a bit better - fixing it in aicclib.php will mean we will need to write an upgrade script to fix any old invalid records as well. I'd also be interested to see if the new code copes with multiple org's in a package - Matteo do you have any packages that use multiple orgs?
          Hide
          Matteo Scaramuccia added a comment -

          Hi Dan,
          not at the moment (read: something that I can share): I'll prepare something this (my) evening/next weekend.

          Show
          Matteo Scaramuccia added a comment - Hi Dan, not at the moment (read: something that I can share): I'll prepare something this (my) evening/next weekend.
          Hide
          Dan Marsden added a comment -

          here's a possible fix that allows "new" AICC packages to be added - it probably won't fix (and may corrupt) existing AICC packages - I need to spend some more time to see if this is the best solution:
          https://github.com/danmarsden/moodle/commit/cbdac55e1e01664ad1c2e65acf378f4c8b771110

          Show
          Dan Marsden added a comment - here's a possible fix that allows "new" AICC packages to be added - it probably won't fix (and may corrupt) existing AICC packages - I need to spend some more time to see if this is the best solution: https://github.com/danmarsden/moodle/commit/cbdac55e1e01664ad1c2e65acf378f4c8b771110
          Hide
          Dan Marsden added a comment -

          here's a first attempt at some code to fix old AICC packages to work as expected - this is untested code that I haven't confirmed as the final solution yet - DON'T APPLY THIS UPGRADE SCRIPT TO A PRODUCTION SITE!!!!!!
          https://github.com/danmarsden/moodle/commit/47c7c542b7e111bd903293174e2bd8bd86045414

          I still need to decide if this is the best way forward - if we decide to do it differently and you apply the above patch to your site it may corrupt your site when/if we apply a different solution.

          Show
          Dan Marsden added a comment - here's a first attempt at some code to fix old AICC packages to work as expected - this is untested code that I haven't confirmed as the final solution yet - DON'T APPLY THIS UPGRADE SCRIPT TO A PRODUCTION SITE!!!!!! https://github.com/danmarsden/moodle/commit/47c7c542b7e111bd903293174e2bd8bd86045414 I still need to decide if this is the best way forward - if we decide to do it differently and you apply the above patch to your site it may corrupt your site when/if we apply a different solution.
          Hide
          Dan Marsden added a comment -

          interesting that no-one else has commented/voted here - Martin have you had many people reporting this as an issue?

          Show
          Dan Marsden added a comment - interesting that no-one else has commented/voted here - Martin have you had many people reporting this as an issue?
          Hide
          Martin Holden added a comment -

          We had a couple of customers reporting the issue, but got round it by using the URL launch fixed in MDL-37394

          Show
          Martin Holden added a comment - We had a couple of customers reporting the issue, but got round it by using the URL launch fixed in MDL-37394
          Hide
          Loic Jeannin added a comment -

          The problem appears for us when the sco objects identifiers are not in the right order : the parent has a greater "id" in the array than the children so the "ksort(result)" in the beginning does not help.
          The following patch is a quick fix for our use case; it assumes that there only is one parent='/' sco and that there is only one level of children, but it may help to implement the real fix.

          https://github.com/jloic/moodle/commit/8a8461f45feb5fcffd35391fe055ea09b6421ddc

          Show
          Loic Jeannin added a comment - The problem appears for us when the sco objects identifiers are not in the right order : the parent has a greater "id" in the array than the children so the "ksort(result)" in the beginning does not help. The following patch is a quick fix for our use case; it assumes that there only is one parent='/' sco and that there is only one level of children, but it may help to implement the real fix. https://github.com/jloic/moodle/commit/8a8461f45feb5fcffd35391fe055ea09b6421ddc
          Hide
          Dan Marsden added a comment -

          Thanks Loic - reliance on the order of scos in the table is something I've wanted to address for a while - we should probably tackle that on a different tracker issue (feel free to create one!)

          Show
          Dan Marsden added a comment - Thanks Loic - reliance on the order of scos in the table is something I've wanted to address for a while - we should probably tackle that on a different tracker issue (feel free to create one!)
          Hide
          Loic Jeannin added a comment -

          Sorry, I assumed the issues were related because of the same symptoms.

          Show
          Loic Jeannin added a comment - Sorry, I assumed the issues were related because of the same symptoms.
          Hide
          Matteo Scaramuccia added a comment -

          Hi Dan,
          I've filed almost a similar issue in MDL-40199 - sorry, my fault not being able to find this issue before!
          To reply at your comment in 04/Apr/13 at least two other persons suffer the same issue:

          Show
          Matteo Scaramuccia added a comment - Hi Dan, I've filed almost a similar issue in MDL-40199 - sorry, my fault not being able to find this issue before! To reply at your comment in 04/Apr/13 at least two other persons suffer the same issue: https://moodle.org/mod/forum/discuss.php?d=228954 https://moodle.org/mod/forum/discuss.php?d=229863
          Hide
          Dan Marsden added a comment -

          sorry - I should have pushed this by now and you wouldn't have needed to spend time on it! - will try to get it upstream this week.

          Show
          Dan Marsden added a comment - sorry - I should have pushed this by now and you wouldn't have needed to spend time on it! - will try to get it upstream this week.
          Hide
          Dan Marsden added a comment -

          Hi Matteo - any chance you could peer review this before I push through? - thanks!

          Show
          Dan Marsden added a comment - Hi Matteo - any chance you could peer review this before I push through? - thanks!
          Hide
          Matteo Scaramuccia added a comment -

          Will look at my evening: BTW, the fix looks very good as per my comments in MDL-40199; just need a bit of extra time to look at the upgrade stage compared to some AICC packages I've. BTW the extra time should not be considered blocking IF we've the chance to get this issue fixed within this week (it's Thursday).

          Show
          Matteo Scaramuccia added a comment - Will look at my evening: BTW, the fix looks very good as per my comments in MDL-40199 ; just need a bit of extra time to look at the upgrade stage compared to some AICC packages I've. BTW the extra time should not be considered blocking IF we've the chance to get this issue fixed within this week (it's Thursday).
          Hide
          Matteo Scaramuccia added a comment -

          Hi Dan,
          yesterday evening I ran out of time: will look this evening if it won't be blocking for you.
          My concern is about https://github.com/danmarsden/moodle/compare/master_MDL-37393#L1R111: we have selected some rows whose organization is empty so we shouldn't use it as a selector for the fist-child-relationship scoes.
          I think it should be:

          +                $scoes = $DB->get_records('scorm_scoes', array('scorm' => $aicc->id, 'parent' => '/', 'organization' => $org->identifier));
          

          but I'd like to have the time to focus on it and not just "5 minutes" as done now.
          Besides maybe it could be done with just a SQL UPDATE statement (for performance reasons?).

          These above are the two things I'd like to focus on.

          Sorry for my being in late,
          Matteo

          Show
          Matteo Scaramuccia added a comment - Hi Dan, yesterday evening I ran out of time: will look this evening if it won't be blocking for you. My concern is about https://github.com/danmarsden/moodle/compare/master_MDL-37393#L1R111: we have selected some rows whose organization is empty so we shouldn't use it as a selector for the fist-child-relationship scoes. I think it should be: + $scoes = $DB->get_records('scorm_scoes', array('scorm' => $aicc->id, 'parent' => '/', 'organization' => $org->identifier)); but I'd like to have the time to focus on it and not just "5 minutes" as done now. Besides maybe it could be done with just a SQL UPDATE statement (for performance reasons?). These above are the two things I'd like to focus on. Sorry for my being in late, Matteo
          Hide
          Dan Marsden added a comment -

          take your time - this has been here for a while - can wait a couple more weeks - better not to cause unintended regressions and review/test it properly than get it in quickly! - thanks for taking a look - I'll try to look closer at your suggestion around identifier sometime next week.

          Show
          Dan Marsden added a comment - take your time - this has been here for a while - can wait a couple more weeks - better not to cause unintended regressions and review/test it properly than get it in quickly! - thanks for taking a look - I'll try to look closer at your suggestion around identifier sometime next week.
          Hide
          Matteo Scaramuccia added a comment -

          Here is a recap of the past comments, to formalize the peer review:

          [Y] Syntax
          [-] Output
          [Y] Whitespace
          [-] Language
          [N] Databases
          [N] Testing
          [-] Security
          [-] Documentation
          [N] Git
          [N] Sanity check
          

          Regarding to the Databases and Sanity Check:

          1. https://github.com/danmarsden/moodle/compare/master_MDL-37393#diff-0, fixing both the first-level child relatioship and cherry picking my commit about STRICT errors:
          2. https://github.com/danmarsden/moodle/compare/master_MDL-37393#diff-1, applying the fix for already uploaded AICC packages:
            The issue here is the incorrect selector, see https://github.com/danmarsden/moodle/compare/master_MDL-37393#L1R111 where $org->organization is empty as per the query definition few lines above
            The question here is also if it will be nicer to use an UPDATE statement instead of a PHP iterator e.g.:
          • Make usage of the current selectors:
            $firstchildssql = "UPDATE
                {scorm_scoes}
            SET
                parent = organization
            WHERE
                    scorm = ?
                AND " . $DB->sql_isempty('scorm_scoes', 'manifest', false, false) . "
                AND organization = ?
                AND parent = '/'";
            $DB->execute($firstchildssql, array($aicc->id, $org->identifier));
            
          • Simplified i.e. no organization iterating, using just $aicc->id:
            $firstchildssql = "UPDATE
                {scorm_scoes}
            SET
                parent = organization
            WHERE
                    scorm = ?
                AND " . $DB->sql_isempty('scorm_scoes', 'manifest', false, false) . "
                AND " . $DB->sql_isnotempty('scorm_scoes', 'organization', false, false) . "
                AND parent = '/'";
            $DB->execute($firstchildssql, array($aicc->id));
            

            This is a bit risky since we cannot have a wide picture of past (1.9-) issues.

          Both the UPDATE statements scope the manifest column and it could be an option to use such scoping as the AICC package signature BUT I'd suggest not to use it but querying the scorm table to select just the AICC packages, as the current code does, or to add ANSI subselects to select just those packages.
          Whether doing everything with:

          • One SQL UPDATE statement
          • Selecting the AICC Packages and using a SQL UPDATE statement
          • Selecting the AICC Packages, selecting ORGs and using a SQL statement
          • Selecting the AICC Packages, selecting ORGs, selecting SCOes and using a SQL statement
            should be driven by the experience and probably the more granularly we act the more fine grained info we could collect in case of specific errors. Not sure what is the preferred Moodle way.

          Regarding to Testing, we should add instructions to populate the Moodle instance with some AICC packages before applying the PR to test both the upgrade stage and the adding of new AICC packages. Among the AICC packages to be used, we could add the one in MDL-33053, https://tracker.moodle.org/secure/attachment/28168/MDL_33053.zip.

          Regarding to Git, just a trivial note: should we use SCORM or AICC as component definition?

          Show
          Matteo Scaramuccia added a comment - Here is a recap of the past comments, to formalize the peer review: [Y] Syntax [-] Output [Y] Whitespace [-] Language [N] Databases [N] Testing [-] Security [-] Documentation [N] Git [N] Sanity check Regarding to the Databases and Sanity Check : https://github.com/danmarsden/moodle/compare/master_MDL-37393#diff-0 , fixing both the first-level child relatioship and cherry picking my commit about STRICT errors: https://github.com/danmarsden/moodle/compare/master_MDL-37393#diff-1 , applying the fix for already uploaded AICC packages: The issue here is the incorrect selector, see https://github.com/danmarsden/moodle/compare/master_MDL-37393#L1R111 where $org->organization is empty as per the query definition few lines above The question here is also if it will be nicer to use an UPDATE statement instead of a PHP iterator e.g.: Make usage of the current selectors: $firstchildssql = "UPDATE {scorm_scoes} SET parent = organization WHERE scorm = ? AND " . $DB->sql_isempty('scorm_scoes', 'manifest', false, false) . " AND organization = ? AND parent = '/'"; $DB->execute($firstchildssql, array($aicc->id, $org->identifier)); Simplified i.e. no organization iterating, using just $aicc->id : $firstchildssql = "UPDATE {scorm_scoes} SET parent = organization WHERE scorm = ? AND " . $DB->sql_isempty('scorm_scoes', 'manifest', false, false) . " AND " . $DB->sql_isnotempty('scorm_scoes', 'organization', false, false) . " AND parent = '/'"; $DB->execute($firstchildssql, array($aicc->id)); This is a bit risky since we cannot have a wide picture of past (1.9-) issues. Both the UPDATE statements scope the manifest column and it could be an option to use such scoping as the AICC package signature BUT I'd suggest not to use it but querying the scorm table to select just the AICC packages, as the current code does, or to add ANSI subselects to select just those packages. Whether doing everything with: One SQL UPDATE statement Selecting the AICC Packages and using a SQL UPDATE statement Selecting the AICC Packages, selecting ORGs and using a SQL statement Selecting the AICC Packages, selecting ORGs , selecting SCOes and using a SQL statement should be driven by the experience and probably the more granularly we act the more fine grained info we could collect in case of specific errors. Not sure what is the preferred Moodle way. Regarding to Testing , we should add instructions to populate the Moodle instance with some AICC packages before applying the PR to test both the upgrade stage and the adding of new AICC packages. Among the AICC packages to be used, we could add the one in MDL-33053 , https://tracker.moodle.org/secure/attachment/28168/MDL_33053.zip . Regarding to Git , just a trivial note: should we use SCORM or AICC as component definition?
          Hide
          Dan Marsden added a comment -

          thanks Matteo - I've just pushed a commit that uses your 2nd sql statement which I liked better - I don't feel comfortable pushing this through for integration without some good testing though - is there anyone watching this issue that could help to test the upgrade script?

          (I'll rebase the commits later - it should be using "SCORM" as the component - not aicc.)

          Show
          Dan Marsden added a comment - thanks Matteo - I've just pushed a commit that uses your 2nd sql statement which I liked better - I don't feel comfortable pushing this through for integration without some good testing though - is there anyone watching this issue that could help to test the upgrade script? (I'll rebase the commits later - it should be using "SCORM" as the component - not aicc.)
          Hide
          Dan Marsden added a comment -

          Bouncing up for integration - Matteo has helped with peer review and Martin Holden has done some testing - thanks Martin/Matteo.

          Note to integrator...
          the master patch has a tidy up of MDL-39239 as well - looks like I bumped version incorrectly in master on MDL-39239 so the master patch tidies this up as well.

          Show
          Dan Marsden added a comment - Bouncing up for integration - Matteo has helped with peer review and Martin Holden has done some testing - thanks Martin/Matteo. Note to integrator... the master patch has a tidy up of MDL-39239 as well - looks like I bumped version incorrectly in master on MDL-39239 so the master patch tidies this up as well.
          Hide
          Dan Marsden added a comment -

          Martin also noticed that restoring an old backup file with AICC packages will result in an incorrect structure being restored - I've created MDL-41153 to resolve that separately so it doesn't hold this up further.

          Show
          Dan Marsden added a comment - Martin also noticed that restoring an old backup file with AICC packages will result in an incorrect structure being restored - I've created MDL-41153 to resolve that separately so it doesn't hold this up further.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Marsden added a comment -

          rebased

          Show
          Dan Marsden added a comment - rebased
          Hide
          Dan Poltawski added a comment -

          Hmm, what happened with the version in MDL-39239? Just wasn't 'branched' higher than 25_stable?

          Show
          Dan Poltawski added a comment - Hmm, what happened with the version in MDL-39239 ? Just wasn't 'branched' higher than 25_stable?
          Hide
          Dan Marsden added a comment -

          yeah - should have been a date bump as it was a master only fix - by only incrementing the last number we prevent bug fixes from going onto stable only correctly...unless I was going mad....

          Show
          Dan Marsden added a comment - yeah - should have been a date bump as it was a master only fix - by only incrementing the last number we prevent bug fixes from going onto stable only correctly...unless I was going mad....
          Hide
          Dan Poltawski added a comment -

          Thanks Dan, Integrated to master, 25 and 24.

          Show
          Dan Poltawski added a comment - Thanks Dan, Integrated to master, 25 and 24.
          Hide
          Adrian Greeve added a comment -

          From what I can see this looks like it is working properly in all of the fixed branches.
          Test passed.

          Show
          Adrian Greeve added a comment - From what I can see this looks like it is working properly in all of the fixed branches. Test passed.
          Hide
          Damyon Wiese added a comment -

          Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week.

          Hurray!

          Show
          Damyon Wiese added a comment - Thanks for your efforts. This issue is one of the outstanding issues that passed all our testing and was accepted into Moodle this week. Hurray!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: