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

When re-entering Multi-SCO SCORM start from the first uncompleted SCO

    Details

    • Testing Instructions:
      Hide

      Using a multi-sco package like the one attached or RuntimeMinimumCalls_SCORM12.zip in mod/scorm/tests/packages.

      • Use the settings current window, student skip content structure = no.
        Enter the scorm, complete some scos, exit.
        Re-enter the scorm you should be taken to the last uncompleted SCO.
      • Use the settings new window, student skip content structure = no.
        Enter the scorm, complete some scos, exit. (or re-enter the previously partially complete scorm)
        On a re-entry you should be taken to the last uncompleted SCO.
      • Use the settings current window, student skip content structure = always.
        Login as a student
        Enter the scorm, complete some scos, exit. (or re-enter the previously partially complete scorm)
        On a re-entry you should be taken to the last uncompleted SCO.
      • Use the settings new window, student skip content structure = always.
        Login as a student
        Enter the scorm, complete some scos, exit. (or re-enter the previously partially complete scorm)
        On a re-entry you should be taken to the last uncompleted SCO.
      • Use the settings current window, student skip content structure = always.
        Login as a teacher or admin user - this will take you to the content structure page as only students are "skipped"
        Enter the scorm, complete some scos, exit. (or re-enter the previously partially complete scorm)
        On a re-entry you should be taken to the last uncompleted SCO.
      Show
      Using a multi-sco package like the one attached or RuntimeMinimumCalls_SCORM12.zip in mod/scorm/tests/packages. Use the settings current window, student skip content structure = no. Enter the scorm, complete some scos, exit. Re-enter the scorm you should be taken to the last uncompleted SCO. Use the settings new window, student skip content structure = no. Enter the scorm, complete some scos, exit. (or re-enter the previously partially complete scorm) On a re-entry you should be taken to the last uncompleted SCO. Use the settings current window, student skip content structure = always. Login as a student Enter the scorm, complete some scos, exit. (or re-enter the previously partially complete scorm) On a re-entry you should be taken to the last uncompleted SCO. Use the settings new window, student skip content structure = always. Login as a student Enter the scorm, complete some scos, exit. (or re-enter the previously partially complete scorm) On a re-entry you should be taken to the last uncompleted SCO. Use the settings current window, student skip content structure = always. Login as a teacher or admin user - this will take you to the content structure page as only students are "skipped" Enter the scorm, complete some scos, exit. (or re-enter the previously partially complete scorm) On a re-entry you should be taken to the last uncompleted SCO.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE, MOODLE_27_STABLE
    • Fixed Branches:
      MOODLE_31_STABLE, MOODLE_32_STABLE
    • Pull 3.1 Branch:
      m31_MDL-46782
    • Pull 3.2 Branch:
      m32_MDL-46782
    • Pull Master Branch:
      master_MDL-46782

      Description

      Hello,

      I've been testing moodle 2.5, 2.6 and 2.7 versions. If I enter a scorm and leave it, the next time I enter it it starts from the scorm first page.

      It is not due to my browser version because I have used several browsers and several versions (included the most updated of any of them) and it happens in any of them.

      I would like to update our moodle to 2.7, but this is a big problem that makes it impossible.

      Thanks in advance.

      Rover

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              danmarsden Dan Marsden added a comment -

              are you suggesting that this package does allow continuing where it left off in an earlier Moodle version? - it's really up to the SCORM package to write to suspend_data and then request information on load and decide how to continue - We have a QA test included in every Moodle release which has passed recently - it also includes a SCORM package that allows this to work. See MDLQA-87 for the details (please don't add comments to that bug it's cloned and used as the master copy during each QA run.)

              Show
              danmarsden Dan Marsden added a comment - are you suggesting that this package does allow continuing where it left off in an earlier Moodle version? - it's really up to the SCORM package to write to suspend_data and then request information on load and decide how to continue - We have a QA test included in every Moodle release which has passed recently - it also includes a SCORM package that allows this to work. See MDLQA-87 for the details (please don't add comments to that bug it's cloned and used as the master copy during each QA run.)
              Hide
              dinky Roberto added a comment - - edited

              Hello.

              I know that this scorm is not saving any suspend data. The only thing that I am suggesting is that scorms like the one I have attached works without any problems in moodle 2.3 and earlier versions as well as other platforms like chamilo. The problem is that we have some scorms like this in our moodle 2.3 platform and we would like to upgrade to the 2.6 or even 2.7 version.

              I will solve this issue by myself because I suspect that these packages will not work on newer versions of moodle any more.

              I have tested the scorm package at MDLQA-87 and it works fine, but the buttons at the bottom of the player are a bit ugly. We have some packages like this too but I have tested one like the one I attached in the message so I apologize to say that player doesn't play well scorm packages, I should have said that it doesn't play well some packages that in earlier versions worked fine.

              Thanks.

              Rover.

              Show
              dinky Roberto added a comment - - edited Hello. I know that this scorm is not saving any suspend data. The only thing that I am suggesting is that scorms like the one I have attached works without any problems in moodle 2.3 and earlier versions as well as other platforms like chamilo. The problem is that we have some scorms like this in our moodle 2.3 platform and we would like to upgrade to the 2.6 or even 2.7 version. I will solve this issue by myself because I suspect that these packages will not work on newer versions of moodle any more. I have tested the scorm package at MDLQA-87 and it works fine, but the buttons at the bottom of the player are a bit ugly. We have some packages like this too but I have tested one like the one I attached in the message so I apologize to say that player doesn't play well scorm packages, I should have said that it doesn't play well some packages that in earlier versions worked fine. Thanks. Rover.
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Roberto,

              thanks for providing information on the Moodle version that operated differently - I've now been able to test this on 2.3 and can see what you mean.

              The old code seemed to set the entry sco as the first incomplete sco it can find rather than relying on the SCORM package to manage this. (it was also buried in some complex convoluted code that is pretty hard to interpret and wasn't documented at all.)

              I don't see an issue with adding this back but I need to look to see if we need to add some form of setting to turn this on/off or to remember if we made this change on purpose.

              If we do add it back I'll create a new QA test and use the package you provided here for testing if that's ok.

              Thanks!

              Show
              danmarsden Dan Marsden added a comment - Hi Roberto, thanks for providing information on the Moodle version that operated differently - I've now been able to test this on 2.3 and can see what you mean. The old code seemed to set the entry sco as the first incomplete sco it can find rather than relying on the SCORM package to manage this. (it was also buried in some complex convoluted code that is pretty hard to interpret and wasn't documented at all.) I don't see an issue with adding this back but I need to look to see if we need to add some form of setting to turn this on/off or to remember if we made this change on purpose. If we do add it back I'll create a new QA test and use the package you provided here for testing if that's ok. Thanks!
              Hide
              danmarsden Dan Marsden added a comment -

              I think this is the relevant part of the SCORM 1.2 Spec that describes this very obscurely.
              "LMSs may adaptively determine sequencing based on the fulfillment of defined prerequisites of learning resources. The progression through learning resources that comprise a particular learning experience may be sequential, non-sequential, user-directed, or adaptive, depending on the capabilities of the LMS"

              Matteo Scaramuccia might have some further thoughts on this too but unless I'm missing something else in the spec I think this is saying that it's optional for the LMS to determine how/which sco is launched.
              At the moment I haven't decided whether this needs to be controllable via a new setting in Moodle to find the first incomplete scorm on re-entry or if we can force all packages to operate this way, I'm also not sure how this will conflict with packages that do implement a method for continuing where you left off separately from this so we'd also need to check the package in MDLQA-87 to see what happens.

              Show
              danmarsden Dan Marsden added a comment - I think this is the relevant part of the SCORM 1.2 Spec that describes this very obscurely. "LMSs may adaptively determine sequencing based on the fulfillment of defined prerequisites of learning resources. The progression through learning resources that comprise a particular learning experience may be sequential, non-sequential, user-directed, or adaptive, depending on the capabilities of the LMS" Matteo Scaramuccia might have some further thoughts on this too but unless I'm missing something else in the spec I think this is saying that it's optional for the LMS to determine how/which sco is launched. At the moment I haven't decided whether this needs to be controllable via a new setting in Moodle to find the first incomplete scorm on re-entry or if we can force all packages to operate this way, I'm also not sure how this will conflict with packages that do implement a method for continuing where you left off separately from this so we'd also need to check the package in MDLQA-87 to see what happens.
              Hide
              dinky Roberto added a comment - - edited

              Hello,

              apart from enabling the continued use of part of our packages and of others, I think it is also very useful for browsers of mobile devices that do not have javascript in order to display the scorm packages correctly because the data stored in the LMS are controlled in the scorm packages using javascript. So it's a way to improve the accessibility of the platform somehow.

              Thanks in advance.

              Rover

              Show
              dinky Roberto added a comment - - edited Hello, apart from enabling the continued use of part of our packages and of others, I think it is also very useful for browsers of mobile devices that do not have javascript in order to display the scorm packages correctly because the data stored in the LMS are controlled in the scorm packages using javascript. So it's a way to improve the accessibility of the platform somehow. Thanks in advance. Rover
              Hide
              dinky Roberto added a comment -

              Hello,

              I forgot to answer your previous question. You can use the package I sent you if you want.

              Thanks

              Show
              dinky Roberto added a comment - Hello, I forgot to answer your previous question. You can use the package I sent you if you want. Thanks
              Hide
              matteo Matteo Scaramuccia added a comment - - edited

              Hi All,
              yes, Dan is right for 1.2: the previous behavior let the user be landed on the "last SCO" that needs user's attention. Since any CMI element is SCO-scoped you can't use e.g. lesson_location or suspend_data to take decisions: first they are content-driven and not understandable by the LMS, second their logic is intra SCO.
              Besides 1.2 didn't have explicit sequencing rules so we could add that logic back (by adding it or fixing the current code), keeping care not to break the existing incomplete SCORM 2004 support.

              For reference: 2.3, https://github.com/moodle/moodle/blob/MOODLE_23_STABLE/mod/scorm/locallib.php#L1421 while master, https://github.com/moodle/moodle/blob/master/mod/scorm/locallib.php#L1489.

              Show
              matteo Matteo Scaramuccia added a comment - - edited Hi All, yes, Dan is right for 1.2: the previous behavior let the user be landed on the "last SCO" that needs user's attention. Since any CMI element is SCO-scoped you can't use e.g. lesson_location or suspend_data to take decisions: first they are content-driven and not understandable by the LMS, second their logic is intra SCO. Besides 1.2 didn't have explicit sequencing rules so we could add that logic back (by adding it or fixing the current code), keeping care not to break the existing incomplete SCORM 2004 support. For reference: 2.3, https://github.com/moodle/moodle/blob/MOODLE_23_STABLE/mod/scorm/locallib.php#L1421 while master, https://github.com/moodle/moodle/blob/master/mod/scorm/locallib.php#L1489 .
              Hide
              taatparya vikram solia added a comment -

              Hi Dan,

              If you can help us identifying the code that needs to be different, we would like to support some older SCORMs for one of our clients. Alternatively, would it be possible to republish the SCORMs in such a way as to work correctly with the current versions.

              Thanks in advance,
              Vikram

              Show
              taatparya vikram solia added a comment - Hi Dan, If you can help us identifying the code that needs to be different, we would like to support some older SCORMs for one of our clients. Alternatively, would it be possible to republish the SCORMs in such a way as to work correctly with the current versions. Thanks in advance, Vikram
              Hide
              taatparya vikram solia added a comment -

              Verified that the issue is still there for Moodle 2.8 and 2.9.

              Show
              taatparya vikram solia added a comment - Verified that the issue is still there for Moodle 2.8 and 2.9.
              Hide
              danmarsden Dan Marsden added a comment -

              Matteo provided a link to the likely location that will require a patch - feel free to submit a patch to help us resolve this. If you do not have the internal development resource to help develop a patch but can fund the time for a developer to look at it feel free to drop me an e-mail. Alternatively someone in the community may find time to work on this one day!

              thanks,

              Show
              danmarsden Dan Marsden added a comment - Matteo provided a link to the likely location that will require a patch - feel free to submit a patch to help us resolve this. If you do not have the internal development resource to help develop a patch but can fund the time for a developer to look at it feel free to drop me an e-mail. Alternatively someone in the community may find time to work on this one day! thanks,
              Hide
              danmarsden Dan Marsden added a comment -

              here's a POC commit that shows where the issue is on the entry page - we need to look at the pop-up/skipcontent structure options and make sure this change doesn't introduce other issues... something for another day:
              https://github.com/danmarsden/moodle/commit/1f864017b562906909eb78f842f96048ad60ffa4

              Show
              danmarsden Dan Marsden added a comment - here's a POC commit that shows where the issue is on the entry page - we need to look at the pop-up/skipcontent structure options and make sure this change doesn't introduce other issues... something for another day: https://github.com/danmarsden/moodle/commit/1f864017b562906909eb78f842f96048ad60ffa4
              Hide
              danmarsden Dan Marsden added a comment -

              here's a patch that should fix this up. I'd like to get some behat tests in here as well but putting this up for peer review for further feedback first.

              Show
              danmarsden Dan Marsden added a comment - here's a patch that should fix this up. I'd like to get some behat tests in here as well but putting this up for peer review for further feedback first.
              Hide
              cibot CiBoT added a comment -

              Fails against automated checks.

              Checked MDL-46782 using repository: git://github.com/danmarsden/moodle.git

              Should these errors be fixed?

              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-46782 using repository: git://github.com/danmarsden/moodle.git master (1 errors / 0 warnings) [branch: master_MDL-46782 | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , Should these errors be fixed?
              Hide
              danmarsden Dan Marsden added a comment -

              Raj found an issue in MDL-52168 with the iframe content check that needs to be added to the behat test here as well.

              Show
              danmarsden Dan Marsden added a comment - Raj found an issue in MDL-52168 with the iframe content check that needs to be added to the behat test here as well.
              Hide
              danmarsden Dan Marsden added a comment -

              nope - that one relates to a different scorm package so this one should be all good.

              Show
              danmarsden Dan Marsden added a comment - nope - that one relates to a different scorm package so this one should be all good.
              Hide
              lameze Simey Lameze added a comment -

              Hi Dan Marsden, thanks for work on this.

              I have reviewed your patch and it looks good to me. Also, kudos for providing a behat test, it passes on your branch and fails without your patch!
              So just to clarify: this regression was introduced on 2.4, have you found which issue/commit number? Would be good link to that issue.

              Anyway, feel free to backport and push for integration review.

              Thanks.

              Show
              lameze Simey Lameze added a comment - Hi Dan Marsden , thanks for work on this. I have reviewed your patch and it looks good to me. Also, kudos for providing a behat test, it passes on your branch and fails without your patch! So just to clarify: this regression was introduced on 2.4, have you found which issue/commit number? Would be good link to that issue. Anyway, feel free to backport and push for integration review. Thanks.
              Hide
              danmarsden Dan Marsden added a comment -

              Thanks!

              Show
              danmarsden Dan Marsden added a comment - Thanks!
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-46782 using repository: git://github.com/danmarsden/moodle.git MOODLE_31_STABLE (1 errors / 0 warnings) [branch: m31_MDL-46782 | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , MOODLE_32_STABLE (1 errors / 0 warnings) [branch: m32_MDL-46782 | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , master (1 errors / 0 warnings) [branch: master_MDL-46782 | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , Should these errors be fixed?
              Hide
              dobedobedoh Andrew Nicols 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
              dobedobedoh Andrew Nicols 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
              cibot CiBoT added a comment -

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

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Show
              cibot CiBoT added a comment - Fails against automated checks. Checked MDL-46782 using repository: git://github.com/danmarsden/moodle.git MOODLE_31_STABLE (1 errors / 0 warnings) [branch: m31_MDL-46782 | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , MOODLE_32_STABLE (1 errors / 0 warnings) [branch: m32_MDL-46782 | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , master (1 errors / 0 warnings) [branch: master_MDL-46782 | CI Job ] phplint (0/0) , phpcs (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , grunt (0/0) , shifter (0/0) , travis (0/0) , mustache (0/0) , Should these errors be fixed?
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks Dan, integrated to master, 32 and 31

              Show
              poltawski Dan Poltawski added a comment - Thanks Dan, integrated to master, 32 and 31
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Test-failing this. We are getting consistent failures in CI servers with this applied.

              Just tried here the --name 'Test completion with all scos' scenario and it fails all the time. Reverting causes it to pass again.

              Ciao

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Test-failing this. We are getting consistent failures in CI servers with this applied. Just tried here the --name 'Test completion with all scos' scenario and it fails all the time. Reverting causes it to pass again. Ciao
              Hide
              danmarsden Dan Marsden added a comment -

              ah, thanks Eloy - that test will need updating. Lines 99 - 103 here will need to be removed:
              https://github.com/moodle/moodle/blob/master/mod/scorm/tests/behat/completion_condition_require_status.feature#L99

              but after doing that the old test will also be covering the new test I've added there, so maybe we should remove the new test and just update the old test? - there seems to be a preference not to add to many behat tests these days? - let me know if that's ok and I'll push a commit.

              Show
              danmarsden Dan Marsden added a comment - ah, thanks Eloy - that test will need updating. Lines 99 - 103 here will need to be removed: https://github.com/moodle/moodle/blob/master/mod/scorm/tests/behat/completion_condition_require_status.feature#L99 but after doing that the old test will also be covering the new test I've added there, so maybe we should remove the new test and just update the old test? - there seems to be a preference not to add to many behat tests these days? - let me know if that's ok and I'll push a commit.
              Hide
              danmarsden Dan Marsden added a comment -

              I've just added commits to the master/3.2 branches to remove the redundant test and fix the existing test to include support for this new change. I'm not sure if we should keep the new test on the 3.1 branch because it's not currently covered? - or if removing the new test was the right change here?

              Show
              danmarsden Dan Marsden added a comment - I've just added commits to the master/3.2 branches to remove the redundant test and fix the existing test to include support for this new change. I'm not sure if we should keep the new test on the 3.1 branch because it's not currently covered? - or if removing the new test was the right change here?
              Hide
              rajeshtaneja Rajesh Taneja added a comment - - edited

              Thanks Dan,

              IMO it would be nice to keep new tests on 31 to get a coverage in 31, better we backport mod/scorm/tests/behat/completion_condition_require_status.feature

              I was looking at this and "Require all scos to return completion status" setting is not available in 31, so I modified mod/scorm/tests/behat/multisco_reentry.feature and kept it as part of 31 branch.

              wip-mdl-46782-m31
              https://github.com/rajeshtaneja/moodle/compare/wip-mdl-46782-m31~1...wip-mdl-46782-m31
              https://github.com/rajeshtaneja/moodle.git wip-mdl-46782-m31

              Show
              rajeshtaneja Rajesh Taneja added a comment - - edited Thanks Dan, IMO it would be nice to keep new tests on 31 to get a coverage in 31, better we backport mod/scorm/tests/behat/completion_condition_require_status.feature I was looking at this and "Require all scos to return completion status" setting is not available in 31, so I modified mod/scorm/tests/behat/multisco_reentry.feature and kept it as part of 31 branch. wip-mdl-46782-m31 https://github.com/rajeshtaneja/moodle/compare/wip-mdl-46782-m31~1...wip-mdl-46782-m31 https://github.com/rajeshtaneja/moodle.git wip-mdl-46782-m31
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks Dan and Raj,

              I've pulled those patches now. Sending back to testing.

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks Dan and Raj, I've pulled those patches now. Sending back to testing.
              Hide
              jaked Jake Dallimore added a comment -

              Tested on master, 32 and 31 and this works as described. Test passed Thanks, Dan.

              Show
              jaked Jake Dallimore added a comment - Tested on master, 32 and 31 and this works as described. Test passed Thanks, Dan.
              Hide
              cibot CiBoT added a comment -

              Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

              Closing as fixed!

              Show
              cibot CiBoT added a comment - Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org. Closing as fixed!

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    13/Mar/17