Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-40223

launch value on mdl_scorm is reused when backup of course is used to make a new course.

    Details

    • Testing Instructions:
      Hide
      Before upgrade.
      1. Break one of your existing scorm pacakges by modifying the scorm->launch field (scorm table, launch param) - set to an integer that is incorrect (like 999999)
      2. Run upgrade then check to make sure launch param has been updated to something more normal (it may not match the same sco-id as previously but as long as it points to the id of the first launchable sco from the mdl_scorm_scoes table and entering the SCORM as a student after the upgrade still works.
      Test of backup/restore.
      1. Create a course.
      2. Add a SCORM activity and upload a SCORM 1.2 content piece.
      3. Add 2 or 3 more of the same.
      4. Go to the SCORM activity and verifies it launches.
      5. Backup the course.
      6. Restore the course as a new course in the system.
      7. Go to the SCORM activity.
      8. Make sure it works as expected.
      9. Go to your database and check the mdl_scorm table's launch value for the new scorm row is different than the launch value for the scorm in the course you backed up.
      Show
      Before upgrade. Break one of your existing scorm pacakges by modifying the scorm->launch field (scorm table, launch param) - set to an integer that is incorrect (like 999999) Run upgrade then check to make sure launch param has been updated to something more normal (it may not match the same sco-id as previously but as long as it points to the id of the first launchable sco from the mdl_scorm_scoes table and entering the SCORM as a student after the upgrade still works. Test of backup/restore. Create a course. Add a SCORM activity and upload a SCORM 1.2 content piece. Add 2 or 3 more of the same. Go to the SCORM activity and verifies it launches. Backup the course. Restore the course as a new course in the system. Go to the SCORM activity. Make sure it works as expected. Go to your database and check the mdl_scorm table's launch value for the new scorm row is different than the launch value for the scorm in the course you backed up.
    • Workaround:
      Hide

      Update the mdl_scorm.launch value to be a unique value. Return to the SCORM activity and the content will launch.

      Show
      Update the mdl_scorm.launch value to be a unique value. Return to the SCORM activity and the content will launch.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull Master Branch:
      master_MDL-40223

      Description

      When you have a course with SCORM activities and you take a backup of that course and then restore to make a new course with that backup. Some content will not launch in the new course. This appears to be due to the launch field on mdl_scorm being the same value as it is in the initial course.

      When I took a look in the DB I found these items share the same launch value in mdl_scorm. I updated the mdl_scorm.launch value to be a unique value. I then went back to the course and went to the SCORM activity and the content launched.

      Before upgrade.
      Break one of your existing scorm pacakges by modifying the scorm->launch field (scorm table, launch param) - set to an integer that is incorrect (like 999999)
      Run upgrade then check to make sure launch param has been updated to something more normal (it may not match the same sco-id as previously but as long as it points to the id of the first launchable sco from the mdl_scorm_scoes table and entering the SCORM as a student after the upgrade still works.

      Test of backup/restore.
      Create a Course
      Add an SCORM activity and upload a SCORM 1.2 content piece
      Add 2 or 3 more of the same
      Go to the SCORM activity and verifies it launches
      Backup the Course
      Restore the Course as a new course in the system
      Go to the SCORM activity
      Content Does not launch

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            danmarsden Dan Marsden added a comment -

            thanks - are there any php errors that occur in the logs? - interesting that this has only just become an issue - it looks like restore has never handled the launch property correctly so probably the GSOC work we did in 2.3 to tidy up the structure of the player might have made it break.

            Show
            danmarsden Dan Marsden added a comment - thanks - are there any php errors that occur in the logs? - interesting that this has only just become an issue - it looks like restore has never handled the launch property correctly so probably the GSOC work we did in 2.3 to tidy up the structure of the player might have made it break.
            Hide
            danmarsden Dan Marsden added a comment -

            here's the first version of this - I also need to write an upgrade script to fix any existing SCORM records with an invalid launch value.

            Show
            danmarsden Dan Marsden added a comment - here's the first version of this - I also need to write an upgrade script to fix any existing SCORM records with an invalid launch value.
            Hide
            danmarsden Dan Marsden added a comment -

            (above patch is untested at this stage)

            Show
            danmarsden Dan Marsden added a comment - (above patch is untested at this stage)
            Hide
            danmarsden Dan Marsden added a comment -

            I've updated the patch to include an upgrade/fix script for existing records and added a patch for 2.5 stable (warning - the 2.5 stable patch is slightly different as the scos in 2.6 are ordered differently than in 2.5)

            If anyone has time to test this or provide feedback on the patch that would be really appreciated.

            Show
            danmarsden Dan Marsden added a comment - I've updated the patch to include an upgrade/fix script for existing records and added a patch for 2.5 stable (warning - the 2.5 stable patch is slightly different as the scos in 2.6 are ordered differently than in 2.5) If anyone has time to test this or provide feedback on the patch that would be really appreciated.
            Hide
            markn Mark Nelson added a comment -

            Hi Dan,

            I tested this by creating SCORM activities before the patch and upgrading to check that the launch value was changed for 2.5 and master and it worked as expected. I also tested restoring after the upgrade and it worked as well.

            Code looks fine. Few minor points -

            1. // Get all scorms that have a launch value that references a sco from a different scorm - please add a full-stop here.
            2. $DB->sql_isnotempty('scorm_scoes', 'launch', false, true); is the true correct at the end? The launch field is an integer, so shouldn't this be false?
            2. if $scoes = $DB->get_records_select('scorm_scoes', $sqlselect, array($scormid), 'sortorder', 'id', 0, 1); why not use get_record_select here, then you won't have to do the current function call. The same applies for the upgrade script. Also, what if this doesn't return anything, won't that break $scorm->launch = $sco->id;?

            Can you update the testing instructions so that it includes an upgrade of existing scorms that have this issue as well?

            Show
            markn Mark Nelson added a comment - Hi Dan, I tested this by creating SCORM activities before the patch and upgrading to check that the launch value was changed for 2.5 and master and it worked as expected. I also tested restoring after the upgrade and it worked as well. Code looks fine. Few minor points - 1. // Get all scorms that have a launch value that references a sco from a different scorm - please add a full-stop here. 2. $DB->sql_isnotempty('scorm_scoes', 'launch', false, true); is the true correct at the end? The launch field is an integer, so shouldn't this be false? 2. if $scoes = $DB->get_records_select('scorm_scoes', $sqlselect, array($scormid), 'sortorder', 'id', 0, 1); why not use get_record_select here, then you won't have to do the current function call. The same applies for the upgrade script. Also, what if this doesn't return anything, won't that break $scorm->launch = $sco->id;? Can you update the testing instructions so that it includes an upgrade of existing scorms that have this issue as well?
            Hide
            danmarsden Dan Marsden added a comment -

            thanks mark
            1 - not sure why codechecker rules didn't pick that up - thanks.
            2 - you're looking at the wrong table - launch in scorm_scoes is a "text" field - launch in "scorm" is int - stupid historical names as they are quite different - launch in scorm is actually sco->id - launch in scorm_scoes is a path to a file.
            3 - not sure why I was doing get_records_select there... possibly a copy paste from somewhere - makes sense to use get_record_select instead thanks

            Show
            danmarsden Dan Marsden added a comment - thanks mark 1 - not sure why codechecker rules didn't pick that up - thanks. 2 - you're looking at the wrong table - launch in scorm_scoes is a "text" field - launch in "scorm" is int - stupid historical names as they are quite different - launch in scorm is actually sco->id - launch in scorm_scoes is a path to a file. 3 - not sure why I was doing get_records_select there... possibly a copy paste from somewhere - makes sense to use get_record_select instead thanks
            Hide
            danmarsden Dan Marsden added a comment -

            also - if the function call doesn't return anything it doesn't change the launch param - we can't fix it as we don't know what it should have been and it's likely to be broken in some other way - if there are scorm packages with this issue we'd need to deal with them on a different bug.

            Show
            danmarsden Dan Marsden added a comment - also - if the function call doesn't return anything it doesn't change the launch param - we can't fix it as we don't know what it should have been and it's likely to be broken in some other way - if there are scorm packages with this issue we'd need to deal with them on a different bug.
            Hide
            danmarsden Dan Marsden added a comment -

            HI Mark - I've updated the branch with those changes - I remember why I was using get_records now - it's because I was avoiding including sortorder and limit in the sql - but it makes sense to just use sql anyway.... - let me know if this looks better - thanks.

            Show
            danmarsden Dan Marsden added a comment - HI Mark - I've updated the branch with those changes - I remember why I was using get_records now - it's because I was avoiding including sortorder and limit in the sql - but it makes sense to just use sql anyway.... - let me know if this looks better - thanks.
            Hide
            markn Mark Nelson added a comment -

            Thanks Dan, I updated the testing instructions. If you can read over them and make sure they are correct that would be great.

            Minor points -

            1. Is WHERE c.id IS null necessary in this query? Is it ever possible for the primary key to be null?
            2. Now that you are using get_record_select there is no real need for the LIMIT 1, as you can pass IGNORE_MULTIPLE to the function. Though, I am not sure which is more effective.
            3. There are 2 lines after "$scorms->close();", only need 1 here.
            4. I would personally add a new line after that if statement in your upgrade.php file before returning true for readability, but just my personal preference so really ignore me if you think it is better the other way.
            5. Is there a reason you removed the 2.5 patch for this? If we do backport to 2.5, should we also for 2.4?

            Thanks!

            Show
            markn Mark Nelson added a comment - Thanks Dan, I updated the testing instructions. If you can read over them and make sure they are correct that would be great. Minor points - Is WHERE c.id IS null necessary in this query? Is it ever possible for the primary key to be null? Now that you are using get_record_select there is no real need for the LIMIT 1, as you can pass IGNORE_MULTIPLE to the function. Though, I am not sure which is more effective. There are 2 lines after "$scorms->close();", only need 1 here. I would personally add a new line after that if statement in your upgrade.php file before returning true for readability, but just my personal preference so really ignore me if you think it is better the other way. Is there a reason you removed the 2.5 patch for this? If we do backport to 2.5, should we also for 2.4? Thanks!
            Hide
            danmarsden Dan Marsden added a comment -

            1 - left join so includes Scorm records that don't have a matching scorm_scoes record against the launch param (so c.id = null) - these are records that really need fixing - scorm->launch references a sco->id that doesn't exist.
            2 - IGNORE_MULTIPLE is mentioned in docs as not reccommended - I presume it just hides the exception.
            3 - only one empty line there - seperates out the logic from the "end" - complies with coding guidelines afaik.
            4 - yup that should be there - missed that one.
            5 - removed 2.5 patch as it is different - (sortorder/id) - easier to peer review and update a single patch - when happy with it I'll backport again.

            thanks!

            Show
            danmarsden Dan Marsden added a comment - 1 - left join so includes Scorm records that don't have a matching scorm_scoes record against the launch param (so c.id = null) - these are records that really need fixing - scorm->launch references a sco->id that doesn't exist. 2 - IGNORE_MULTIPLE is mentioned in docs as not reccommended - I presume it just hides the exception. 3 - only one empty line there - seperates out the logic from the "end" - complies with coding guidelines afaik. 4 - yup that should be there - missed that one. 5 - removed 2.5 patch as it is different - (sortorder/id) - easier to peer review and update a single patch - when happy with it I'll backport again. thanks!
            Hide
            markn Mark Nelson added a comment -

            I still don't see how an entry could be entered into that table with no ID as it is an auto-incremented primary key. Any ways, Dan, please feel free to submit for integration, though I would suggest squashing the commits into one as having a commit with the message "tidy up after peer review" makes the Moodle history messy.

            Show
            markn Mark Nelson added a comment - I still don't see how an entry could be entered into that table with no ID as it is an auto-incremented primary key. Any ways, Dan, please feel free to submit for integration, though I would suggest squashing the commits into one as having a commit with the message "tidy up after peer review" makes the Moodle history messy.
            Hide
            danmarsden Dan Marsden added a comment - - edited

            thanks Mark - here's a brief explanation of the joins:

            table A

            id scoid
            1 5
            2 7
            3 8

            TABLE B

            sco name
            5 sco1
            7 sco2

            A normal Join on those 2 tables would return

            id scoid sco name
            1 5 5 sco1
            2 7 7 sco2

            A Left Join on those 2 tables returns:

            id scoid sco name
            1 5 5 sco1
            2 7 7 sco2
            3 8 null null

            so although "sco" in the Table B is the PK because we are doing a LEFT join on Table A - it can be null

            Show
            danmarsden Dan Marsden added a comment - - edited thanks Mark - here's a brief explanation of the joins: table A id scoid 1 5 2 7 3 8 TABLE B sco name 5 sco1 7 sco2 A normal Join on those 2 tables would return id scoid sco name 1 5 5 sco1 2 7 7 sco2 A Left Join on those 2 tables returns: id scoid sco name 1 5 5 sco1 2 7 7 sco2 3 8 null null so although "sco" in the Table B is the PK because we are doing a LEFT join on Table A - it can be null
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Dan,

            Looking at the restore changes - I initially thought the process_scorm function would be the best place to restore the mapping - but the list of scos has not been processed at that point, so you can't. (Nasty cyclic data).

            Integrated to 24, 25, 26 and master.

            Show
            damyon Damyon Wiese added a comment - Thanks Dan, Looking at the restore changes - I initially thought the process_scorm function would be the best place to restore the mapping - but the list of scos has not been processed at that point, so you can't. (Nasty cyclic data). Integrated to 24, 25, 26 and master.
            Hide
            danmarsden Dan Marsden added a comment -

            thanks Damyon

            Show
            danmarsden Dan Marsden added a comment - thanks Damyon
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Please AMEND this asap. It's breaking MSSQL/ORACLE. Harcoded LIMIT clause!
            (I'm failing this now)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Please AMEND this asap. It's breaking MSSQL/ORACLE. Harcoded LIMIT clause! (I'm failing this now)
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            (note that at least 2 uses have been detected, one in restore, the other in upgrade!)

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - (note that at least 2 uses have been detected, one in restore, the other in upgrade!)
            Hide
            danmarsden Dan Marsden added a comment -

            thanks Eloy - fixing now.

            Show
            danmarsden Dan Marsden added a comment - thanks Eloy - fixing now.
            Hide
            danmarsden Dan Marsden added a comment -

            heh - now I remember why I was using a records call instead of a "record" call - you can't pass a limit in _record style calls - Eloy is there a helper function for using limit? - or do I have to use a get_records function call then pop the first item off the array like I was doing earlier?

            Show
            danmarsden Dan Marsden added a comment - heh - now I remember why I was using a records call instead of a "record" call - you can't pass a limit in _record style calls - Eloy is there a helper function for using limit? - or do I have to use a get_records function call then pop the first item off the array like I was doing earlier?
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            get_records/recordset_xxxx() have limitfrom, limitnum params.

            singular ones have "strictness" param.

            In any case, perhaps you should be using record_exists() + set_field() if you are not interested in results. That's another alternative.

            Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - get_records/recordset_xxxx() have limitfrom, limitnum params. singular ones have "strictness" param. In any case, perhaps you should be using record_exists() + set_field() if you are not interested in results. That's another alternative. Ciao
            Hide
            danmarsden Dan Marsden added a comment -

            updated all the branches with a fix - NOTE: patches for 2.6/master are different from the patch for 2.5/2.4 (sortorder field is "id" in 2.5/2.4 and sortorder is "sortorder" in master/2.6) - thanks Eloy

            Show
            danmarsden Dan Marsden added a comment - updated all the branches with a fix - NOTE: patches for 2.6/master are different from the patch for 2.5/2.4 (sortorder field is "id" in 2.5/2.4 and sortorder is "sortorder" in master/2.6) - thanks Eloy
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            thumbs up, looks perfect, thanks for the quick patch!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - thumbs up, looks perfect, thanks for the quick patch!
            Hide
            damyon Damyon Wiese added a comment -

            Thanks for the fix Dan,

            I've pushed that to all branches.

            Back to testing.

            Show
            damyon Damyon Wiese added a comment - Thanks for the fix Dan, I've pushed that to all branches. Back to testing.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Works as described.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Works as described. Thanks
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            ...
            But still, I thank you, for you made me stronger…

            Stronger as the beast that roar in the wild
            Stronger as the storm across the ocean
            Stronger as the diamond that won’t break
            Stronger enough to take all heart aches….

            I thank you my friend, for you made me stronger…

            ---- Juneah Landicho

            Closing as fixed. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - ... But still, I thank you, for you made me stronger… Stronger as the beast that roar in the wild Stronger as the storm across the ocean Stronger as the diamond that won’t break Stronger enough to take all heart aches…. I thank you my friend, for you made me stronger… ---- Juneah Landicho Closing as fixed. Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  13/Jan/14