Moodle
  1. Moodle
  2. MDL-43541

scorm.launch should represent the first launch-able item

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.7, 2.5.3, 2.6, 2.7
    • Fix Version/s: 2.5.5, 2.6.2
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      Patch for 2.5 is slightly different than 2.6/master - this must be tested on 2.5 stable as well.

      Add the attached scorm package to a course

      Use the following settings:

        • Display package: New Window
        • Display course structure in player: To the side
        • Student skip content structure: Always
          Enter the SCORM as a student (not admin/teacher)
          Make sure popup is generated and SCORM loads "SCORM Diagnostic SCO ORG2" (shown in title of TOC)

      Test 2:
      Enter the SCORM as an admin - it will display the "view.php" page and show the usual enter button - hit the enter button to enter the SCORM and make sure it loads "SCORM Diagnostic SCO ORG2" (shown in title of TOC) on first entry.

      Show
      Patch for 2.5 is slightly different than 2.6/master - this must be tested on 2.5 stable as well. Add the attached scorm package to a course Use the following settings: Display package: New Window Display course structure in player: To the side Student skip content structure: Always Enter the SCORM as a student (not admin/teacher) Make sure popup is generated and SCORM loads "SCORM Diagnostic SCO ORG2" (shown in title of TOC) Test 2: Enter the SCORM as an admin - it will display the "view.php" page and show the usual enter button - hit the enter button to enter the SCORM and make sure it loads "SCORM Diagnostic SCO ORG2" (shown in title of TOC) on first entry.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull 2.6 Branch:
      m26_MDL-43541
    • Pull Master Branch:
      master_MDL-43541

      Description

      The launch column in the scorm table represents, in case of SCORM, the first launch-able "organization".
      This leads to issues when a "simple" user launches a SCORM package configured to be displayed in a new window, always skipping the content structure page (among the latest discussions see e.g. https://moodle.org/mod/forum/discuss.php?d=251252 and https://moodle.org/mod/forum/discuss.php?d=251952) while this doesn't happen when viewed in the same window (tech.: scorm_simple_play mitigates the issue for teachers, managers and administrators).

      A similar issue applies to AICC packages too.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Matteo Scaramuccia added a comment - - edited

            Hi Dan,
            it seems it's time to change the launch value in the database or to hack the code in order to correctly discover the first launch-able item (see https://github.com/moodle/moodle/blob/669755a423b73f6db285114332271207af05ce6b/mod/scorm/view.php#L74).

            What do you think? My vote goes for the first option: see MDL-40223, while in MDL-37529 I was trying to do something like the required hack, see 88830dd071.

            Show
            Matteo Scaramuccia added a comment - - edited Hi Dan, it seems it's time to change the launch value in the database or to hack the code in order to correctly discover the first launch-able item (see https://github.com/moodle/moodle/blob/669755a423b73f6db285114332271207af05ce6b/mod/scorm/view.php#L74 ). What do you think? My vote goes for the first option: see MDL-40223 , while in MDL-37529 I was trying to do something like the required hack, see 88830dd071 .
            Hide
            Matteo Scaramuccia added a comment -

            In the mean time, I've refactored my code proposal.
            I'll extend it to the supported branch as well as add the required upgrade step.

            Show
            Matteo Scaramuccia added a comment - In the mean time, I've refactored my code proposal. I'll extend it to the supported branch as well as add the required upgrade step.
            Show
            Matteo Scaramuccia added a comment - Added https://github.com/scara/moodle/commit/f11a9104b570d164ceea531f0504bce1aa9e3c9e#diff-2a9b3d8e64715a5201c426403c991c17R921 .
            Hide
            Dan Marsden added a comment -

            Thanks Matteo - just to confirm - you're seeing an issue with this when skipview is enabled - even after the last patches landed? - how do we get a SCORM package into an invalid state?

            Show
            Dan Marsden added a comment - Thanks Matteo - just to confirm - you're seeing an issue with this when skipview is enabled - even after the last patches landed? - how do we get a SCORM package into an invalid state?
            Hide
            Dan Marsden added a comment -

            modified your patch a bit to match the other code that checks for launch param - would be good if you could sanity check it as well?

            Show
            Dan Marsden added a comment - modified your patch a bit to match the other code that checks for launch param - would be good if you could sanity check it as well?
            Hide
            Dan Marsden added a comment -

            pushing up for peer review.

            NOTE: patch for 2.5 is slightly different - 2.6 orders scos using the new sortorder field - 2.5 orders using the id field.

            Show
            Dan Marsden added a comment - pushing up for peer review. NOTE: patch for 2.5 is slightly different - 2.6 orders scos using the new sortorder field - 2.5 orders using the id field.
            Hide
            Matteo Scaramuccia added a comment -

            Hi Dan,
            sorry for my late reply (busy for the first week after holidays):

            1. your PR seems to missing a "critical" part: "AND id > ?" could be relevant especially for SCORM where we could define a default organization among multiple organizations. The launch-able SCO must be selected within the default organization and the way the manifest is parsed configures the first item within the organization as the first item added just after the one representing the organization. Not sure who's using multiple organizations but this missing could lead to an incorrect first launch-able SCO;
            2. the only missing part is the DB update because we updated the value in the database when restoring a course but not now for the already published packages.
              What I was thinking is to create the update stage using the same query used to select the launch-able SCO when publishing.

            I've also added an improvement to always provide the launch-able SCO regardless the "launch" parameters mode to avoid mis-matching behaviors. The code could be improved in other areas to create a common function to select the first lauch-able SCO in those areas where the missing of that parameter is resolved by finding the missing value (see e.g. here): I thought this could be something to be addressed in master when a SCORM code cleanup will be optionally performed.

            Matteo

            Show
            Matteo Scaramuccia added a comment - Hi Dan, sorry for my late reply (busy for the first week after holidays): your PR seems to missing a "critical" part: "AND id > ?" could be relevant especially for SCORM where we could define a default organization among multiple organizations. The launch-able SCO must be selected within the default organization and the way the manifest is parsed configures the first item within the organization as the first item added just after the one representing the organization. Not sure who's using multiple organizations but this missing could lead to an incorrect first launch-able SCO; the only missing part is the DB update because we updated the value in the database when restoring a course but not now for the already published packages. What I was thinking is to create the update stage using the same query used to select the launch-able SCO when publishing. I've also added an improvement to always provide the launch-able SCO regardless the "launch" parameters mode to avoid mis-matching behaviors. The code could be improved in other areas to create a common function to select the first lauch-able SCO in those areas where the missing of that parameter is resolved by finding the missing value (see e.g. here ): I thought this could be something to be addressed in master when a SCORM code cleanup will be optionally performed. Matteo
            Hide
            Dan Marsden added a comment -

            Thanks Matteo.
            1 - in 2.6 and higher the scoes are sorted by a new sortorder field - so using the order of id's wouldn't work. Also - the query sorts the records by the sortorder (or id if 2.5) field and returns the first item so it's already finding the first record - theoretically this should pick up the right sco as the default will be listed first right?

            2 - we've already run a query through the db to update existing records with the lauch param (uses the same sql I've used in my patch above) - we could update this code to run again at the same time if you think it's really needed?

            Show
            Dan Marsden added a comment - Thanks Matteo. 1 - in 2.6 and higher the scoes are sorted by a new sortorder field - so using the order of id's wouldn't work. Also - the query sorts the records by the sortorder (or id if 2.5) field and returns the first item so it's already finding the first record - theoretically this should pick up the right sco as the default will be listed first right? 2 - we've already run a query through the db to update existing records with the lauch param (uses the same sql I've used in my patch above) - we could update this code to run again at the same time if you think it's really needed?
            Hide
            Matteo Scaramuccia added a comment -

            Hi Dan,
            here are my replies:

            1. What about a SCORM package like the one below?

              ...
                  <organizations default="ORG2"> 
                      <organization identifier="ORG1"> 
                          <title>SCORM Test</title>
                          <item identifier="I_SCO11" identifierref="SCO11">
                              <title>SCO 1.1</title>
                              <adlcp:masteryscore>75</adlcp:masteryscore>
                          </item>
                      </organization>
                      <organization identifier="ORG2"> 
                          <title>SCORM Test</title>
                          <item identifier="I_SCO21" identifierref="SCO21">
                              <title>SCO 2.1</title>
                              <adlcp:masteryscore>75</adlcp:masteryscore>
                          </item>
                      </organization>
                      <organization identifier="ORG3> 
                          <title>SCORM Test</title>
                          <item identifier="I_SCO31" identifierref="SCO31">
                              <title>SCO 3.1</title>
                              <adlcp:masteryscore>75</adlcp:masteryscore>
                          </item>
                      </organization>
                  </organizations>
              ...
              

              The launch-able item to be inserted into scorm->launch should be the identity of I_SCO21 and the scorm->launch value actually set by scorm_parse_scorm() is the id of the default organization: given this example what is the best strategy - sortorder included - to select that SCO? That's the reason why we should add the current value of scorm->launch in the query charged to select the first launch-able SCO within the given organization defined by scorm->launch and nearest to it since it is the first child of that organization;

            2. yep, in MDL-40223, available since 1.5 months: we could rely on that but excluding those that published/updated their packages after having updated to their Moodle instance. Besides that query - IF I'm right on point 1 - is affected by the same issue in the rare case of packages with multiple organizations.

            I could be wrong - especially with the DB - and I'll recheck the code by testing it in the next evenings.

            Show
            Matteo Scaramuccia added a comment - Hi Dan, here are my replies: What about a SCORM package like the one below? ... <organizations default="ORG2"> <organization identifier="ORG1"> <title>SCORM Test</title> <item identifier="I_SCO11" identifierref="SCO11"> <title>SCO 1.1</title> <adlcp:masteryscore>75</adlcp:masteryscore> </item> </organization> <organization identifier="ORG2"> <title>SCORM Test</title> <item identifier="I_SCO21" identifierref="SCO21"> <title>SCO 2.1</title> <adlcp:masteryscore>75</adlcp:masteryscore> </item> </organization> <organization identifier="ORG3> <title>SCORM Test</title> <item identifier="I_SCO31" identifierref="SCO31"> <title>SCO 3.1</title> <adlcp:masteryscore>75</adlcp:masteryscore> </item> </organization> </organizations> ... The launch-able item to be inserted into scorm->launch should be the identity of I_SCO21 and the scorm->launch value actually set by scorm_parse_scorm() is the id of the default organization: given this example what is the best strategy - sortorder included - to select that SCO? That's the reason why we should add the current value of scorm->launch in the query charged to select the first launch-able SCO within the given organization defined by scorm->launch and nearest to it since it is the first child of that organization; yep, in MDL-40223 , available since 1.5 months: we could rely on that but excluding those that published/updated their packages after having updated to their Moodle instance. Besides that query - IF I'm right on point 1 - is affected by the same issue in the rare case of packages with multiple organizations. I could be wrong - especially with the DB - and I'll recheck the code by testing it in the next evenings.
            Hide
            Dan Marsden added a comment -

            maybe I'm still missing something there? - the organisation id shouldn't be selected by that sql as we are only looking for scos that have a valid "launch" field - org records will have an empty launch field won't they? - the sql looks for the first valid sco that has a launch value. Is that not occuring?

            Show
            Dan Marsden added a comment - maybe I'm still missing something there? - the organisation id shouldn't be selected by that sql as we are only looking for scos that have a valid "launch" field - org records will have an empty launch field won't they? - the sql looks for the first valid sco that has a launch value. Is that not occuring?
            Hide
            Matteo Scaramuccia added a comment -

            Hi Dan,
            it the current code design, scorm_parse_*(), evaluate $scorm->launch as the identity of the organization (not launch-able), and then we need to "fix" that value with the first launch-able SCO whithin the given organization. In the example above, the current code should return I_SCO11 - the first available launch-able SCO - instead of the required I_SCO21.

            Show
            Matteo Scaramuccia added a comment - Hi Dan, it the current code design, scorm_parse_*() , evaluate $scorm->launch as the identity of the organization (not launch-able), and then we need to "fix" that value with the first launch-able SCO whithin the given organization. In the example above, the current code should return I_SCO11 - the first available launch-able SCO - instead of the required I_SCO21 .
            Hide
            Dan Marsden added a comment -

            Ah, now I see the "default" org issue - I wasn't looking closely enough there.
            We should probably shift this logic into scorm_parse_scorm and scorm_parse_aicc instead of trying to "fix" it elsewhere. Hopefully we can also do it without needing to hit the db as we already have the scoes in memory at that point.

            I'll try to take a look at this later in the week (unless you beat me to it!)

            Show
            Dan Marsden added a comment - Ah, now I see the "default" org issue - I wasn't looking closely enough there. We should probably shift this logic into scorm_parse_scorm and scorm_parse_aicc instead of trying to "fix" it elsewhere. Hopefully we can also do it without needing to hit the db as we already have the scoes in memory at that point. I'll try to take a look at this later in the week (unless you beat me to it!)
            Hide
            Dan Marsden added a comment -

            here's a new untested attempt that does scorm_parse_scorm only: https://github.com/danmarsden/moodle/compare/moodle:master...master_MDL-43541-v2

            Show
            Dan Marsden added a comment - here's a new untested attempt that does scorm_parse_scorm only: https://github.com/danmarsden/moodle/compare/moodle:master...master_MDL-43541-v2
            Hide
            Dan Marsden added a comment -

            new version of the code for peer review - Matteo any chance you could take another look at this one?

            Show
            Dan Marsden added a comment - new version of the code for peer review - Matteo any chance you could take another look at this one?
            Hide
            CiBoT added a comment -

            Results for MDL-43541

            • Remote repository: git://github.com/danmarsden/moodle.git
            Show
            CiBoT added a comment - Results for MDL-43541 Remote repository: git://github.com/danmarsden/moodle.git Remote branch master_ MDL-43541 -v2 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/939 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/939/artifact/work/smurf.html
            Hide
            Matteo Scaramuccia added a comment -

            Hi Dan,
            yes, will do in the week-end.

            Show
            Matteo Scaramuccia added a comment - Hi Dan, yes, will do in the week-end.
            Hide
            Matteo Scaramuccia added a comment -

            Added a SCORM 1.2 test package, using the Diagnostic SCO Package as the base and modified to host 3 organizations (ORG1, ORG2, ORG3), each composed by 3 items (SCO<ORGno>1, SCO<ORGno>2, SCO<ORGno>3), where the second has been marked as "default".

            When published in a "clean" master the student falls into the issue and the scorm.launch points to the item in scorm_scoes representing the default organization, ORG2.

            The expected result is to launch the item whose item@identifier is SCO21 and title SCORM Diagnostic SCO 2.1.

            Applying the patch and re-published the package as another activity to test the fix on both the situations: package being just published with the patch and package already published...

            Show
            Matteo Scaramuccia added a comment - Added a SCORM 1.2 test package, using the Diagnostic SCO Package as the base and modified to host 3 organizations ( ORG1 , ORG2 , ORG3 ), each composed by 3 items ( SCO<ORGno>1 , SCO<ORGno>2 , SCO<ORGno>3 ), where the second has been marked as "default". When published in a "clean" master the student falls into the issue and the scorm.launch points to the item in scorm_scoes representing the default organization, ORG2 . The expected result is to launch the item whose item@identifier is SCO21 and title SCORM Diagnostic SCO 2.1 . Applying the patch and re-published the package as another activity to test the fix on both the situations: package being just published with the patch and package already published...
            Hide
            Matteo Scaramuccia added a comment -

            Hi Dan,
            the patch doesn't apply to the current master, 183515da233fd9d96a64d94e215300851183a049. Using d5a04809 this is what happens:

            curl https://github.com/danmarsden/moodle/compare/moodle:master...master_MDL-43541-v2.patch | git apply -
              % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                             Dload  Upload   Total   Spent    Left  Speed
            100  7927  100  7927    0     0  11569      0 --:--:-- --:--:-- --:--:-- 1548k
            error: patch failed: mod/scorm/version.php:25
            error: mod/scorm/version.php: patch does not apply
            

            Could you rebase the PR and/or test your current PR with the package I've uploaded?

            Your new PR is cleaner and nicely safer () on updating a SCORM activity when using a SCORM package (i.e.: https://github.com/danmarsden/moodle/commit/d5a04809c77d387bd3ef5c42190a359cc0b41712#diff-2063da475cf1dad6e7baf71f9ab6be2aR683):

            1. I expect it will work on brand new SCORM activities as well as on updating existing ones;
            2. I guess there will be an issue on packages with multiple organizations, already published and subjected to the update process: it seems to me that with the current code the first launch-able item will be, using the test package, SCORM Diagnostic SCO 1.1 instead of SCORM Diagnostic SCO 2.1.

            HTH,
            Matteo

            Show
            Matteo Scaramuccia added a comment - Hi Dan, the patch doesn't apply to the current master , 183515da233fd9d96a64d94e215300851183a049 . Using d5a04809 this is what happens: curl https://github.com/danmarsden/moodle/compare/moodle:master...master_MDL-43541-v2.patch | git apply - % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 7927 100 7927 0 0 11569 0 --:--:-- --:--:-- --:--:-- 1548k error: patch failed: mod/scorm/version.php:25 error: mod/scorm/version.php: patch does not apply Could you rebase the PR and/or test your current PR with the package I've uploaded? Your new PR is cleaner and nicely safer ( ) on updating a SCORM activity when using a SCORM package (i.e.: https://github.com/danmarsden/moodle/commit/d5a04809c77d387bd3ef5c42190a359cc0b41712#diff-2063da475cf1dad6e7baf71f9ab6be2aR683): I expect it will work on brand new SCORM activities as well as on updating existing ones; I guess there will be an issue on packages with multiple organizations, already published and subjected to the update process: it seems to me that with the current code the first launch-able item will be, using the test package, SCORM Diagnostic SCO 1.1 instead of SCORM Diagnostic SCO 2.1 . HTH, Matteo
            Hide
            Matteo Scaramuccia added a comment -

            [Y] Syntax
            [Y] Whitespace
            [-] Output
            [-] Language
            [N] Databases
            [N] Testing (instructions and automated tests)
            [-] Security
            [-] Documentation
            [N] Git
            [-] Third party code
            [N] Sanity check
            

            I've performed the rebase manually via:

            $ git fetch https://github.com/danmarsden/moodle master_MDL-43541-v2
            $ git checkout -b dan_master_MDL-43541-v2 FETCH_HEAD
            $ git rebase master dan_master_MDL-43541-v2
            

            fixing a merge conflict in mod/scorm/version.php. Then updated and found an issue using MySQL 5.5:

            Debug info: Column 'launch' in where clause is ambiguous
            SELECT s.*
            FROM mdl_scorm s
            LEFT JOIN mdl_scorm_scoes c ON s.launch = c.id
            WHERE (launch = '') 
            [array (
            )]
            Error code: dmlreadexception
            Stack trace:
            line 443 of /lib/dml/moodle_database.php: dml_read_exception thrown
            line 953 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
            line 212 of /mod/scorm/db/upgrade.php: call to mysqli_native_moodle_database->get_recordset_sql()
            line 671 of /lib/upgradelib.php: call to xmldb_scorm_upgrade()
            line 395 of /lib/upgradelib.php: call to upgrade_plugins_modules()
            line 1581 of /lib/upgradelib.php: call to upgrade_plugins()
            line 427 of /admin/index.php: call to upgrade_noncore()
            

            Fixed using:

            diff --git a/mod/scorm/db/upgrade.php b/mod/scorm/db/upgrade.php
            index 2c40f98..77a8ef8 100644
            --- a/mod/scorm/db/upgrade.php
            +++ b/mod/scorm/db/upgrade.php
            @@ -208,7 +208,7 @@ function xmldb_scorm_upgrade($oldversion) {
                     $sql = "SELECT s.*
                              FROM {scorm} s
                         LEFT JOIN {scorm_scoes} c ON s.launch = c.id
            -                WHERE ".$DB->sql_isempty('scorm_scoes', 'launch', false, true);
            +                WHERE ".$DB->sql_isempty('scorm_scoes', 'c.launch', false, true);
                     $scorms = $DB->get_recordset_sql($sql);
                     foreach ($scorms as $scorm) {
                         // Find the first launchable sco for this SCORM.
            

            After the successful run of the update stage, the scorm.launch has been updated but not as expected for a correct solution: it points to SCORM Diagnostic SCO 1.1 instead of SCORM Diagnostic SCO 2.1.

            The same applies when adding the same test package as a new SCORM Activity: the item is selected within the first organization and not within the default organization.

            I suggest to include also the update stage into the testing instructions and maybe to reduce the length of first (and only) comment line in the git commit message.

            Show
            Matteo Scaramuccia added a comment - [Y] Syntax [Y] Whitespace [-] Output [-] Language [N] Databases [N] Testing (instructions and automated tests) [-] Security [-] Documentation [N] Git [-] Third party code [N] Sanity check I've performed the rebase manually via: $ git fetch https://github.com/danmarsden/moodle master_MDL-43541-v2 $ git checkout -b dan_master_MDL-43541-v2 FETCH_HEAD $ git rebase master dan_master_MDL-43541-v2 fixing a merge conflict in mod/scorm/version.php . Then updated and found an issue using MySQL 5.5 : Debug info: Column 'launch' in where clause is ambiguous SELECT s.* FROM mdl_scorm s LEFT JOIN mdl_scorm_scoes c ON s.launch = c.id WHERE (launch = '') [array ( )] Error code: dmlreadexception Stack trace: line 443 of /lib/dml/moodle_database.php: dml_read_exception thrown line 953 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end() line 212 of /mod/scorm/db/upgrade.php: call to mysqli_native_moodle_database->get_recordset_sql() line 671 of /lib/upgradelib.php: call to xmldb_scorm_upgrade() line 395 of /lib/upgradelib.php: call to upgrade_plugins_modules() line 1581 of /lib/upgradelib.php: call to upgrade_plugins() line 427 of /admin/index.php: call to upgrade_noncore() Fixed using: diff --git a/mod/scorm/db/upgrade.php b/mod/scorm/db/upgrade.php index 2c40f98..77a8ef8 100644 --- a/mod/scorm/db/upgrade.php +++ b/mod/scorm/db/upgrade.php @@ -208,7 +208,7 @@ function xmldb_scorm_upgrade($oldversion) { $sql = "SELECT s.* FROM {scorm} s LEFT JOIN {scorm_scoes} c ON s.launch = c.id - WHERE ".$DB->sql_isempty('scorm_scoes', 'launch', false, true); + WHERE ".$DB->sql_isempty('scorm_scoes', 'c.launch', false, true); $scorms = $DB->get_recordset_sql($sql); foreach ($scorms as $scorm) { // Find the first launchable sco for this SCORM. After the successful run of the update stage, the scorm.launch has been updated but not as expected for a correct solution: it points to SCORM Diagnostic SCO 1.1 instead of SCORM Diagnostic SCO 2.1 . The same applies when adding the same test package as a new SCORM Activity: the item is selected within the first organization and not within the default organization. I suggest to include also the update stage into the testing instructions and maybe to reduce the length of first (and only) comment line in the git commit message.
            Hide
            Dan Marsden added a comment -

            Thanks Matteo - I'm actually ok with the update command setting the first launchable sco for existing incorrect packages - I don't expect there will be many of those pacakges around and at least this will allow them to be launched. I definitely don't want to write an upgrade script that parses the imsmanifest for all scorms but we could look at the existing launch param - but was that setting correctly to the default org in the old code? - will need to test.

            Your statement "The same applies when adding the same test package as a new SCORM Activity" - do you mean that the SCORM isn't setting the right sco even when creating a new package? - I'll test that again as I thought that was working.

            Show
            Dan Marsden added a comment - Thanks Matteo - I'm actually ok with the update command setting the first launchable sco for existing incorrect packages - I don't expect there will be many of those pacakges around and at least this will allow them to be launched. I definitely don't want to write an upgrade script that parses the imsmanifest for all scorms but we could look at the existing launch param - but was that setting correctly to the default org in the old code? - will need to test. Your statement "The same applies when adding the same test package as a new SCORM Activity" - do you mean that the SCORM isn't setting the right sco even when creating a new package? - I'll test that again as I thought that was working.
            Hide
            Matteo Scaramuccia added a comment -

            Hi Dan,
            the old code correctly selects the default organization item.
            BTW, yes, adding a new SCORM activity with the new code will select the first launch-able SCO among all the organizations but not the first launch-able SCO within the default organization.

            Show
            Matteo Scaramuccia added a comment - Hi Dan, the old code correctly selects the default organization item. BTW, yes, adding a new SCORM activity with the new code will select the first launch-able SCO among all the organizations but not the first launch-able SCO within the default organization.
            Hide
            Dan Marsden added a comment -

            pushed a fix that makes it work when adding a new activity - still looking at the db upgrade to see if there's a nice way to handle it without too many nested queries.

            Show
            Dan Marsden added a comment - pushed a fix that makes it work when adding a new activity - still looking at the db upgrade to see if there's a nice way to handle it without too many nested queries.
            Hide
            Dan Marsden added a comment -

            added code to upgrade broken scorm packages to include logic to select first sco inside default org and falls back to first launchable sco if it can't find one.

            Show
            Dan Marsden added a comment - added code to upgrade broken scorm packages to include logic to select first sco inside default org and falls back to first launchable sco if it can't find one.
            Hide
            CiBoT added a comment -

            Results for MDL-43541

            • Remote repository: git://github.com/danmarsden/moodle.git
            Show
            CiBoT added a comment - Results for MDL-43541 Remote repository: git://github.com/danmarsden/moodle.git Remote branch master_ MDL-43541 -v2 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1137 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1137/artifact/work/smurf.html
            Hide
            Matteo Scaramuccia added a comment -

            [N] Syntax
            [Y] Whitespace
            [-] Output
            [-] Language
            [Y] Databases
            [Y] Testing (instructions and automated tests)
            [-] Security
            [-] Documentation
            [Y] Git
            [-] Third party code
            [Y] Sanity check
            

            I'm fine with the current PR but some code cleanups:

            1. [TRIVIAL] ($defaultorgid == 0): why not empty()? It has been defined to be equal to 0 and that variable makes sense when "not set" i.e. empty() here looks better to me;
            2. isset($defaultorgid): why not !empty()? Now it is always true since it has been already defined as 0;
            3. [MINOR] $scorm->launch = 0;: I would initialize that value before starting with the discovery logic to be fine with all conditional branches. now the code leave it undefined when empty($scoes) is true. I suggest to fix it even if the missing conditional branch means that the package has issues by its own to avoid strict warnings.

            Among the trivial improvements:

            1. Take care of what code checker warns about. I'm not sure if it is required but, in case, it should be good to add it as a separate commit.

            I didn't test AICC packages and external HACP: the code seems fine and should not add regressions.

            HTH,
            Matteo

            Show
            Matteo Scaramuccia added a comment - [N] Syntax [Y] Whitespace [-] Output [-] Language [Y] Databases [Y] Testing (instructions and automated tests) [-] Security [-] Documentation [Y] Git [-] Third party code [Y] Sanity check I'm fine with the current PR but some code cleanups: [TRIVIAL] ($defaultorgid == 0) : why not empty() ? It has been defined to be equal to 0 and that variable makes sense when "not set" i.e. empty() here looks better to me; isset($defaultorgid) : why not !empty() ? Now it is always true since it has been already defined as 0 ; [MINOR] $scorm->launch = 0; : I would initialize that value before starting with the discovery logic to be fine with all conditional branches. now the code leave it undefined when empty($scoes) is true . I suggest to fix it even if the missing conditional branch means that the package has issues by its own to avoid strict warnings. Among the trivial improvements: Take care of what code checker warns about . I'm not sure if it is required but, in case, it should be good to add it as a separate commit. I didn't test AICC packages and external HACP: the code seems fine and should not add regressions. HTH, Matteo
            Hide
            Matteo Scaramuccia added a comment -

            Hi Dan,
            yesterday I missed to mention a point: what about using upgrade_set_timeout() - e.g. 10 for really slow systems. Note: that function will impose it to 60, being the minimum value - for each iteration? Worst case is four queries per activity: it would help in excluding timeouts in those systems with a large number of SCORM activities.

            Show
            Matteo Scaramuccia added a comment - Hi Dan, yesterday I missed to mention a point: what about using upgrade_set_timeout() - e.g. 10 for really slow systems. Note: that function will impose it to 60 , being the minimum value - for each iteration ? Worst case is four queries per activity: it would help in excluding timeouts in those systems with a large number of SCORM activities.
            Hide
            Dan Marsden added a comment -

            thanks Matteo - some good feedback there, have made those changes - I was ignoring some of the pre-existing PHPDoc stuff but I suppose it's a good time to start tidying it up!

            Show
            Dan Marsden added a comment - thanks Matteo - some good feedback there, have made those changes - I was ignoring some of the pre-existing PHPDoc stuff but I suppose it's a good time to start tidying it up!
            Hide
            Matteo Scaramuccia added a comment -

            Glad to read that it helped .

            1. Still there the (isset($defaultorgid): is it a miss or a choice?
            2. Is mod/assignment/db/upgrade.php an "error" here, being not related to this issue?
            Show
            Matteo Scaramuccia added a comment - Glad to read that it helped . Still there the (isset($defaultorgid) : is it a miss or a choice? Is mod/assignment/db/upgrade.php an "error" here, being not related to this issue?
            Hide
            CiBoT added a comment -

            Results for MDL-43541

            • Remote repository: git://github.com/danmarsden/moodle.git
            Show
            CiBoT added a comment - Results for MDL-43541 Remote repository: git://github.com/danmarsden/moodle.git Remote branch master_ MDL-43541 -v2 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1207 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1207/artifact/work/smurf.html
            Hide
            Dan Marsden added a comment -

            thanks - fixed those, will generate stable branch fixes now.

            Show
            Dan Marsden added a comment - thanks - fixed those, will generate stable branch fixes now.
            Hide
            CiBoT added a comment -

            Results for MDL-43541

            • Remote repository: git://github.com/danmarsden/moodle.git
            Show
            CiBoT added a comment - Results for MDL-43541 Remote repository: git://github.com/danmarsden/moodle.git Remote branch master_ MDL-43541 -v2 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/1363 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/1363/artifact/work/smurf.html
            Hide
            Matteo Scaramuccia added a comment -

            Hi Dan,
            just seen another a minor minor issue: why $sqlselect = 'scorm = ? AND sortorder >= ? AND... here and $sqlselect = 'scorm = ? AND sortorder > ? AND... here?
            Is the difference about the equality a missing? It should be strictly greater than - Note: the equality doesn't hurt due to the $DB->sql_isnotempty() constraint - or simply other reasons behind that difference?

            Matteo

            Show
            Matteo Scaramuccia added a comment - Hi Dan, just seen another a minor minor issue: why $sqlselect = 'scorm = ? AND sortorder >= ? AND... here and $sqlselect = 'scorm = ? AND sortorder > ? AND... here ? Is the difference about the equality a missing? It should be strictly greater than - Note: the equality doesn't hurt due to the $DB->sql_isnotempty() constraint - or simply other reasons behind that difference? Matteo
            Hide
            Dan Marsden added a comment -

            thanks - fixed to make it consistent.

            Show
            Dan Marsden added a comment - thanks - fixed to make it consistent.
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Dan Poltawski added a comment -

            Hi Dan,

            Two trivial things:

            • In the phpdocs, we attribute the copyright to the original author of file rather than assign it to Martin - looks like that was Roberto Pinna.
            • Please don't share version numbers between master and 26 - I think you've experienced problems with this before. They need to diverge.
            Show
            Dan Poltawski added a comment - Hi Dan, Two trivial things: In the phpdocs, we attribute the copyright to the original author of file rather than assign it to Martin - looks like that was Roberto Pinna. Please don't share version numbers between master and 26 - I think you've experienced problems with this before. They need to diverge.
            Hide
            Dan Marsden added a comment -

            doh - fixed thanks!

            Show
            Dan Marsden added a comment - doh - fixed thanks!
            Hide
            Dan Poltawski added a comment -

            Thanks Dan, integrated to master, 25 and 26

            Show
            Dan Poltawski added a comment - Thanks Dan, integrated to master, 25 and 26
            Hide
            Rajesh Taneja added a comment -

            Thanks for fixing this Dan,

            Works fine in 25, 26 and master...

            Passing...

            Show
            Rajesh Taneja added a comment - Thanks for fixing this Dan, Works fine in 25, 26 and master... Passing...
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I claim to be a simple individual
            liable to err like any other fellow mortal.
            I own, however, that I have humility enough
            to confess my errors and to retrace my steps.

            Mahatma Gandhi

            Your awesome code has met upstream, closing, thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - I claim to be a simple individual liable to err like any other fellow mortal. I own, however, that I have humility enough to confess my errors and to retrace my steps. Mahatma Gandhi Your awesome code has met upstream, closing, thanks!
            Hide
            Matteo Scaramuccia added a comment -

            Just for reference: the update stage seems to cause issues on SQL Server, https://moodle.org/mod/forum/discuss.php?d=256069.
            If confirmed, a new separate issue will be opened.

            Show
            Matteo Scaramuccia added a comment - Just for reference: the update stage seems to cause issues on SQL Server, https://moodle.org/mod/forum/discuss.php?d=256069 . If confirmed, a new separate issue will be opened.

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: